Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setns() is hard to call safely on Go 1.9 and earlier #17

Closed
bboreham opened this issue Jun 29, 2016 · 9 comments · Fixed by #30
Closed

Setns() is hard to call safely on Go 1.9 and earlier #17

bboreham opened this issue Jun 29, 2016 · 9 comments · Fixed by #30

Comments

@bboreham
Copy link

bboreham commented Jun 29, 2016

For details see moby/libnetwork#1113 or weaveworks/weave#2388 (comment)

Broadly, Setns() only affects the current Linux thread, but the Go runtime can create a new thread while you have changed namespace, so now any operation elsewhere in the program can suddenly switch to the other namespace as Go schedules goroutines onto the new thread.

The only safe thing to do seems to be to exec a new process to do nothing except the task we want to achieve in the alternate namespace, to call runtime.LockOSThread while doing the work, and then to exit the process.

@vishvananda
Copy link
Owner

Yes this is a challenge, but I'm not sure a great way to fix it. One option is to do what the netlink library does, which is to lock the thread for the duration of the netlink socket create. Once the socket is open in the proper namespace, you don't need to setns to run commands against it.

I don't know if there is much else useful that can be done without actually changing the way that the go runtime works. Suggestions happily accepted.

@bboreham
Copy link
Author

lock the thread for the duration of the netlink socket create

That doesn't help at all. The problem is that new threads can be created, and inherit the namespace from the "locked" thread.

Yes, I believe the real fix needs to go into the Go runtime. I posted this thinking (a) maybe someone does know of a way to address the issue, or (b) the code in this repo should carry a warning.

@vishvananda
Copy link
Owner

Ah, I didn't fully understand the issue. I didn't realize the go runtime can start new threads after the initial start of the process. Is this a recent addition or has it been that way since the beginning? It seems like we do need some kind of runtime construct to prevent this from occurring or ensuring that go always starts new threads in the global namespace.

@vishvananda
Copy link
Owner

vishvananda commented Jun 29, 2016

or maybe a change to go so that it cannot start new threads from the context of a locked thread? In any case, I can definitely add a warning to the library.

@bboreham
Copy link
Author

I don't believe this is a new issue. Some of the comments on golang/go#1435 (open 5 years) are relevant, e.g. golang/go#1435 (comment).

I agree with the idea that Go could avoid starting threads as children of a locked thread, but I have no idea how easy that is, or if it breaks something more fundamental in Go.

@bboreham
Copy link
Author

Related: golang/go#20676

@brb
Copy link
Contributor

brb commented Feb 21, 2018

Go 1.10 is out which has fixed most of the problems: https://golang.org/doc/go1.10#runtime. However, keep in mind, that one problem still exists - if you spawn a new goroutine from a locked one, the new routine can be scheduled on any thread, thus creating a new goroutine from a locked one is not safe (worth mentioning in README.md).

To fix #17, you could add the // +build go1.10 pragma to netns_linux.go to prevent users from tripping over.

@vishvananda
Copy link
Owner

vishvananda commented Feb 21, 2018 via email

@bboreham bboreham changed the title Setns() is hard to call safely Setns() is hard to call safely on Go 1.9 and earlier Oct 21, 2020
@bboreham
Copy link
Author

This only affects really old versions so I'll close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants