namespaces: How to enter them in a foolproof way

[egernst]

From @sboeuf on February 27, 2018 20:3

Following discussion on #613 and implementation #615, I will try to summarize what are the issues we're facing with Golang and what are the options here.
We would like to enter any namespaces from virtcontainers (or any code written in Go) and be able to execute some callbacks inside a set of defined namespaces. The reason of why we need to enter those namespaces does not need to be detailed here.

Now, let's say we want to execute some code in a set of namespaces, we can use the package nsenter implemented in the PR #615, which will basically save the current set of namespaces, then enter the targeted namespaces, execute the callback, and switch back to the saved(original) namespaces. But doing so, we have 2 drawbacks that could end up in unwanted behaviors:

• If the callback defines a go routine, the code running into this routine will be running in a different thread (TID will be different), meaning it won't run in the expected set of namespaces.
• If we're using a version < Go 1.10, we might hit a very unlikely issue (but still have a chance), which is inherent to Golang itself. If the code executed from the callback might take too much time, Golang might decide to create a new thread for executing this code, even if runtime.LockOSThread() has been previously called, leading our code to run in the wrong namespaces. This is obviously a Golang bug that has been fixed in Go 1.10.

Additionally to those potential issues, there is a main constraint that cannot be solved by Golang since this is multithreaded: the ability to enter mount or user namespaces. Following discussions here, here and here give some input.

So now, what should we do ? Well having C code is the solution to the multi-threading problem, and this will solve both the drawbacks and the limitations explained above. So why don't we get started ?
Well, it's not as easy as it seems, because we cannot only define a C function called from Go code, this is already too late, you're already multi-threaded at that time.

The solution would be to invoke a C constructor like this:

void __attribute__((constructor)) init_func(int argc, const char **argv)

from our Go code. This constructor is always running before any piece of Go code, ensuring about the single-threading here. In order to reach this code from a Go function, we need to re-execute ourselves with a snippet like this:

package main

/*
#cgo CFLAGS: -Wall
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <unistd.h>

__attribute__((constructor)) static void constructor_map(int argc, const char **argv) {
	fprintf(stdout, "TEST 0\n");
	if (argc > 2) {
		fprintf(stdout, "TEST 1\n");
		exit(0);
	}
	fprintf(stdout, "TEST 2\n");
}
*/
import "C"
import "os"
import "os/exec"

func main() {
	cmd := exec.Command("/proc/self/exe", "init", "test")
	cmd.Stdout = os.Stdout
	cmd.Run()
}

From this code, if you replace the call to fprintf(stdout, "TEST 1\n"); with some code entering namespaces, you can enter any namespaces with the guarantee you won't end up in a non expected namespace. Basically, after entering the set of namespaces, we would need to execute our callback, assuring this would be running in the right namespaces. BUT, cause there is always one, I am not sure we can provide a Go callback through this so that it gets executed after the code entering the namespaces. I did some researches here and there and tried a few things that didn't work, so I am getting skeptical on this possibility. But I think this needs more investigation.
Based on libcontainer code, one way to do this would be to open a pipe between the Go code and the C code so that we can communicate what needs to be done, but again I am not sure about the ability to pass a function pointer here.

Now if we want to solve this more easily by saying that our package will spawn a new binary into the expected namespaces, things get easier. This means we would need to pass a bunch of const char[] info about the binary path and its argument through the parameter list of our re-execution. But notice that in this case, spawning our VM into the right network namespace would need to be implemented at the govmm level, that is the level where we deal with binary path and parameters.

Copied from original issue: containers/virtcontainers#644

[egernst]

From @sboeuf on February 27, 2018 20:4

cc @sameo @mcastelino @amshinde @devimc @grahamwhaley @jodh-intel @jcvenegas @egernst

[egernst]

From @sboeuf on February 28, 2018 21:56

I have been able to pass the address of the function defined in the parent address space with this simple code here:

package main

/*
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <unistd.h>

__attribute__((constructor)) static void constructor_map(int argc, const char **argv) {
	fprintf(stdout, "TEST 0\n");
	if (argc > 3) {
		fprintf(stdout, "argv[3] = %s\n", argv[3]);
		long addr = (long)strtol(argv[3], NULL, 0);
		fprintf(stdout, "addr = %lx\n", addr);
		char* (*goCallback)(void) = (char*(*)(void))addr;

		fprintf(stdout, "TEST 1\n");
		char* ret = goCallback();
		fprintf(stdout, "goCallback ret = %c%c%c%c\n", ret[0], ret[1], ret[2], ret[3]);
		fprintf(stdout, "TEST 2\n");
		exit(0);
	}
	fprintf(stdout, "TEST 3\n");
}
*/
import "C"
import "os"
import "os/exec"
import "fmt"
import "syscall"

func goCallback() string {
	return "t3st"
}

func main() {
	fmt.Printf("%p\n", goCallback)
	addr := fmt.Sprintf("%p", goCallback)

	cmd := exec.Command("/proc/self/exe", "init", "test", addr)
	cmd.Stdout = os.Stdout
	cmd.SysProcAttr = &syscall.SysProcAttr{
		Cloneflags: syscall.CLONE_VM,
	}
	cmd.Run()
}

Output:

TEST 0
TEST 3
0x4a3b40
TEST 0
argv[3] = 0x4a3b40
addr = 4a3b40
TEST 1
goCallback ret = t3st
TEST 2

Unfortunately, if my function goCallback() invoke code such as:

func goCallback() string {
	return fmt.Sprintf("%s", "t3st")
}

relying on some Go imported package, then I get a segfault from the child process:

kernel: exe[31802]: segfault at 10 ip 00000000004a3b49 sp 00007ffeaf6753d8 error 4 in go_callback_from_c_2[400000+149000]

resulting in this output:

TEST 0
TEST 3
0x4a3b40
TEST 0
argv[3] = 0x4a3b40
addr = 4a3b40
TEST 1

This might be solved by the definition of the function in a separate package built as a shared C library, but I think we're going too far here, and also that we won't be able to have a generic package handling any type of Go code from the C code.
Please let me know if you think about something...

Anyway, that being said, we should at least be able to create a package spawning new processes based on their binary path and their list of argument. This should not be too complicated to implement and we could cover the case where we want to spawn the shim or any binary into a specific set of namespaces.

[egernst]

From @sboeuf on February 28, 2018 22:13

As a side note, using the ability to export a callback between Go and C seems impossible in this very specific case (I have tried...). The reason is that we need the constructor to be called so that we ensure we are in single threaded context, leading us to a situation where we have not loaded the Go context yet. This means any export is not yet defined. And this way, we cannot reach the callback that is supposed to be exported.

[egernst]

From @sboeuf on March 5, 2018 19:49

@mcastelino @sameo any feedback on this ?

[egernst]

From @amshinde on March 5, 2018 22:38

@sboeuf I had a very brief look at the nsenter package from libcontainer last week. The way they claim is that C constructor code is executed for every Cmd.Start call. They pass the clone flags with the help of a pipe. The parent spawns a child and then a grandchild (to enter PID namespace as well). (I suppose the grandchild would then go on to execute the shim)
I havent given it a try yet, but may be it would be worth playing with this code to see how it actually works.

Here are the tests that show how the code is supposed to work:
https://github.com/opencontainers/runc/blob/master/libcontainer/nsenter/nsenter_test.go

[egernst]

From @sboeuf on March 6, 2018 0:44

Another side note here from discussions with @amshinde. Given the fact that we won't be able to call a Go callback from a C constructor, we should have an hybrid solution here to cover all the cases we need.

What we need
Enter namespaces safely from a Go program so that we can either spawn a new process or execute some generic Go code from a specific set of namespaces.

What we could do

• Use the current nsenter package to execute any Go code into cgroup, ipc, network, pid or uts namespaces. This is valid for any Go version, but only considered as 100% safe in case of Go 1.10.
• Use a new package that we would call nsenter_c to enter any namespace (including mount and user), but limited to the execution of a binary inside this set of namespaces. This would fit perfectly for cases where we would expect the shim to run in a specific mount or user namespace, but this would not cover generic Go code to be executed inside those namespaces.

This is really the best solution I can think of, given all my researches on that.

Weird thing that could work for a pure C solution
In order to cover the case of handling also Go functions from the C code, we could build a c-shared library using go build -o mygolib.so -buildmode=c-shared mygolib.go. This would embed some known functions like scanNetwork() (if we think about scanning a the network from inside a netns for instance), that we could call from the C constructor code, based on the argument passed through argv[]. This would work because the C constructor would know about the Go function at this time, compared to the callback defined from the code itself.

[WeiZhang555]

In #623 , cli/oci.go still tries to enter netns with Golang codes:

420 // enterNetNS is free from any call to a go routine, and it calls
421 // into runtime.LockOSThread(), meaning it won't be executed in a
422 // different thread than the one expected by the caller.
423 func enterNetNS(netNSPath string, cb func() error) error {
424     if netNSPath == "" {
425         return cb()
426     }
427
428     goruntime.LockOSThread()
429     defer goruntime.UnlockOSThread()
430
431     currentNS, err := ns.GetCurrentNS()
432     if err != nil {
433         return err
434     }
435     defer currentNS.Close()
436
437     targetNS, err := ns.GetNS(netNSPath)
438     if err != nil {
439         return err
440     }
441
442     if err := targetNS.Set(); err != nil {
443         return err
444     }
445     defer currentNS.Set()
446
447     return cb()
448 }

This is not safe when Golang version is <1.10, so I suggest we should bump our runtime Golang version to > 1.10?

@sboeuf @jodh-intel

[sboeuf]

@WeiZhang555 oh yes I forgot about this... So I guess > 1.10 is the right answer here ;)

[amshinde]

Yup, remember discussing this discussion :) We should go with golang >= 1.10, since namespace setns issue has been resolved in 1.10.

[WeiZhang555]

ping @jodh-intel , can you do the Golang bump again? I remembered recently you bumped it to 1.9.x ?

[jodh-intel]

I'm happy to do that, but nobody has mentioned details of the fix in 1.10 yet, which would be very useful to put into the commit 😄

Trying to trace down the fix isn't that easy - I've ended up with a bunch of github and gerrit issues. Could someone either paste the canonical link to the fix (or could it be golang/go@2595fe7 maybe?)

[caoruidong]

@jodh-intel I think you're right. containerd/cri#144 also point to golang/go#20676 which fixed by golang/go@2595fe7

[mcastelino]

Adding a pointer to the release note https://golang.org/doc/go1.10#runtime

[amshinde]

The changes have to do with changes to LockOSThread and UnlockOSThread.
This article has all the relevant git commits and a good explanation:
https://www.weave.works/blog/linux-namespaces-golang-followup

[WeiZhang555]

The build flag sounds great in the weavework article。

// +build go1.10

This can restrict the minimum Golang version for runtime, and we have a good reason to ignore old Golang version compatibility now.

[WeiZhang555]

File a PR to fix this issue: https://github.com/kata-containers/runtime/compare/master...WeiZhang555:golang-1.10?expand=1

Need help from CI, so mark it as WIP

[bergwolf]

If the callback defines a go routine, the code running into this routine will be running in a different thread (TID will be different), meaning it won't run in the expected set of namespaces.

Do we actually have any code that does this? It won't help to solve the problem even if we bump required golang version. So the only option might be carefully making sure that we do not have such kind of code.

If we're using a version < Go 1.10, we might hit a very unlikely issue (but still have a chance), which is inherent to Golang itself. If the code executed from the callback might take too much time, Golang might decide to create a new thread for executing this code, even if runtime.LockOSThread() has been previously called, leading our code to run in the wrong namespaces. This is obviously a Golang bug that has been fixed in Go 1.10.

Just out of curiosity, which golang issue are you referring to, @sboeuf ? From the referenced link https://golang.org/doc/go1.10#runtime, I don't find any description talking about the "locked goroutine changing to a new thread" bug. The described changed behavior with LockOSThread is to require paired LockOSThread/UnlockOSThread calls so that they can be used in a library, and it does not seem to affect us since we only use one level of such locking.

The only related issue I can find is golang/go#20676, which makes sure new threads created by golang do not inherit the private states of a LockOSThreaded thread.

[WeiZhang555]

@bergwolf

The only related issue I can find is golang/go#20676, which makes sure new threads created by golang do not inherit the private states of a LockOSThreaded thread.

Yes, I think this is the issue. And the release note didn't mention it very clearly.

But the issue is real, the blog listed above: https://www.weave.works/blog/linux-namespaces-golang-followup and https://www.weave.works/blog/linux-namespaces-and-go-don-t-mix described the issue very clearly, we once ran into this issue in docker before. It's critical and I think we'd better bump the Golang version as it includes the fix(though not tested locally)

[bergwolf]

@WeiZhang555 I agree it is a serious issue for us and so important that we should bump required go versions. But IIUC, it is not running a locked goroutine in a different OS thread, which is another (more) serious golang bug.

[sboeuf]

@bergwolf the kata-runtime is locked from the very beginning, which ensure locking of the thread from the beginning.