Adds forkexec library that supports user namespaces in Go. #150

mrunalp · 2014-08-08T20:01:15Z

This PR adds a library that could be used to clone into user namespaces and write uid/gid mappings.
(This is based on joint effort between me and crosbymichael).

Docker-DCO-1.1-Signed-off-by: Mrunal Patel [email protected] (github: mrunalp)

mrunalp · 2014-08-08T20:02:00Z

ping @crosbymichael
Could you test please to make sure that I am not missing any races and this actually works! :)

tianon · 2014-08-08T20:51:21Z

forklib/forklib.go

+	"unsafe"
+)
+
+func BeforeFork()


Would it be worth noting in a comment here that these come from forklib.c?

Sure will do.

crosbymichael · 2014-08-08T22:39:10Z

Yes, i would add some comments to the exported functions of the package.

crosbymichael · 2014-08-08T22:40:13Z

forklib/forklib.go

+	var wstatus WaitStatus
+
+	sys := attr.Sys
+	/*


Is this supposed to be commented out?

Fixed to match what is done in exec_unix.go

crosbymichael · 2014-08-08T22:48:33Z

I don't know about the name. Maybe something like "github.com/libcontainer/forkexec" or just fork?

Just some ideas

mrunalp · 2014-08-08T23:11:00Z

@crosbymichael Yeah, wasn't 100% sure on the name so just picked one to get a PR going. I don't mind forkexec.

mrunalp · 2014-08-08T23:20:38Z

Renamed to forkexec (till we finalize a name) and added some comments.

philips · 2014-08-11T22:25:53Z

forkexec/forkexec.go

+	var err1 Errno
+	var wstatus WaitStatus
+
+	sys := attr.Sys


isn't this a potentially unsafe ref if attr is nil?

I will fix it. Thanks for reviewing!

Sent from my iPhone

On Aug 11, 2014, at 3:25 PM, Brandon Philips [email protected] wrote:

In forkexec/forkexec.go:

+// These functions are defined in forkexec.c
+func BeforeFork()
+func AfterFork()
+
+var zeroProcAttr ProcAttr
+var zeroSysProcAttr SysProcAttr
+
+// This is similar to the standard go library implementation of forkExec with the addition
+// of capability to write UID/GID mappings for user namespaces.
+func ForkExecNew(argv0 string, argv []string, attr *ProcAttr, uidMappings, gidMappings []IdMap) (pid int, err error) {

var p [2]int

var n int

var err1 Errno

var wstatus WaitStatus

sys := attr.Sys
isn't this a potentially unsafe ref if attr is nil?

—
Reply to this email directly or view it on GitHub.

philips · 2014-08-11T22:45:19Z

lgtm besides the possible bug and dropping the BSD code.

Docker-DCO-1.1-Signed-off-by: Mrunal Patel <[email protected]> (github: mrunalp)

mrunalp · 2014-08-12T22:03:28Z

Addressed @philips comments.

philips · 2014-08-12T22:16:21Z

lgtm

vmarmol · 2014-08-13T15:36:17Z

LGTM

rjnagal · 2014-08-13T15:52:08Z

LGTM

crosbymichael · 2014-08-13T17:33:38Z

I'm running the example but I'm getting a panic now:

fatal error: split stack after fork

runtime stack:
runtime.throw(0x5654ee)
        /root/go/src/pkg/runtime/panic.c:520 +0x69
runtime.newstack()
        /root/go/src/pkg/runtime/stack.c:679 +0x48
runtime.morestack()
        /root/go/src/pkg/runtime/asm_amd64.s:228 +0x61

goroutine 16 [running]:
github.com/docker/libcontainer/forkexec.AfterFork()
        /root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.c:10 +0x13 fp=0x7f707e949b78 sp=0x7f707e949b70
github.com/docker/libcontainer/forkexec.forkAndExecInChild(0xc208000180, 0xc208000190, 0x2, 0x2, 0xc20803c018, 0x1, 0x1, 0x0, 0x0, 0x7f707e949f00, ...)
        /root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:157 +0x187 fp=0x7f707e949c48 sp=0x7f707e949b78
github.com/docker/libcontainer/forkexec.ForkExecNew(0x4d4530, 0x7, 0x7f707e949e90, 0x1, 0x1, 0x7f707e949f00, 0x7f707e949e44, 0x1, 0x1, 0x7f707e949e38, ...)
        /root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:77 +0x49f fp=0x7f707e949d98 sp=0x7f707e949c48
main.main()
        /root/development/gocode/src/github.com/docker/libcontainer/forkexec/usernstest/main.go:34 +0x346 fp=0x7f707e949f50 sp=0x7f707e949d98
runtime.main()
        /root/go/src/pkg/runtime/proc.c:247 +0x11a fp=0x7f707e949fa8 sp=0x7f707e949f50
runtime.goexit()
        /root/go/src/pkg/runtime/proc.c:1445 fp=0x7f707e949fb0 sp=0x7f707e949fa8
created by _rt0_go
        /root/go/src/pkg/runtime/asm_amd64.s:97 +0x120

goroutine 17 [runnable]:
runtime.MHeap_Scavenger()
        /root/go/src/pkg/runtime/mheap.c:507
runtime.goexit()
        /root/go/src/pkg/runtime/proc.c:1445

goroutine 18 [runnable]:
bgsweep()
        /root/go/src/pkg/runtime/mgc0.c:1976
runtime.goexit()
        /root/go/src/pkg/runtime/proc.c:1445

goroutine 19 [runnable]:
runfinq()
        /root/go/src/pkg/runtime/mgc0.c:2606
runtime.goexit()
        /root/go/src/pkg/runtime/proc.c:1445

mrunalp · 2014-08-13T18:07:40Z

Hmm. Is it with unpatched go?

Sent from my iPhone

On Aug 13, 2014, at 10:33 AM, Michael Crosby [email protected] wrote:

I'm running the example but I'm getting a panic now:

fatal error: split stack after fork

runtime stack:
runtime.throw(0x5654ee)
/root/go/src/pkg/runtime/panic.c:520 +0x69
runtime.newstack()
/root/go/src/pkg/runtime/stack.c:679 +0x48
runtime.morestack()
/root/go/src/pkg/runtime/asm_amd64.s:228 +0x61

goroutine 16 [running]:
github.com/docker/libcontainer/forkexec.AfterFork()
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.c:10 +0x13 fp=0x7f707e949b78 sp=0x7f707e949b70
github.com/docker/libcontainer/forkexec.forkAndExecInChild(0xc208000180, 0xc208000190, 0x2, 0x2, 0xc20803c018, 0x1, 0x1, 0x0, 0x0, 0x7f707e949f00, ...)
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:157 +0x187 fp=0x7f707e949c48 sp=0x7f707e949b78
github.com/docker/libcontainer/forkexec.ForkExecNew(0x4d4530, 0x7, 0x7f707e949e90, 0x1, 0x1, 0x7f707e949f00, 0x7f707e949e44, 0x1, 0x1, 0x7f707e949e38, ...)
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:77 +0x49f fp=0x7f707e949d98 sp=0x7f707e949c48
main.main()
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/usernstest/main.go:34 +0x346 fp=0x7f707e949f50 sp=0x7f707e949d98
runtime.main()
/root/go/src/pkg/runtime/proc.c:247 +0x11a fp=0x7f707e949fa8 sp=0x7f707e949f50
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445 fp=0x7f707e949fb0 sp=0x7f707e949fa8
created by _rt0_go
/root/go/src/pkg/runtime/asm_amd64.s:97 +0x120

goroutine 17 [runnable]:
runtime.MHeap_Scavenger()
/root/go/src/pkg/runtime/mheap.c:507
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445

goroutine 18 [runnable]:
bgsweep()
/root/go/src/pkg/runtime/mgc0.c:1976
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445

goroutine 19 [runnable]:
runfinq()
/root/go/src/pkg/runtime/mgc0.c:2606
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445
—
Reply to this email directly or view it on GitHub.

philips · 2014-08-13T18:11:39Z

@mrunalp There is a Go runtime patch required? I didn't see this mentioned in the PR.

crosbymichael · 2014-08-13T18:13:23Z

No, no Go patch required

mrunalp · 2014-08-13T18:20:22Z

No, this is a workaround so we don't need to patch go. Might have issues so would be good if folks test this. I tried on 3 vms / machines and worked for me. But with the panic that crobymichael saw, this may not work out :(

Sent from my iPhone

On Aug 13, 2014, at 11:11 AM, Brandon Philips [email protected] wrote:

@mrunalp There is a Go runtime patch required? I didn't see this mentioned in the PR.

—
Reply to this email directly or view it on GitHub.

mrunalp · 2014-08-13T19:25:29Z

@crosbymichael Tested some more. This works with fedora default go 1.2.2 but fails like it did for you when I used golang 1.3 compiled from source. I guess back to pushing the patch upstream.

mrunalp · 2014-08-13T19:27:42Z

@vmarmol Could you help get some attention/discussion around -- https://groups.google.com/forum/#!searchin/golang-nuts/User$20Namespaces/golang-nuts/J60fxDVSaco/EU_qJGqTNUkJ ?
I believe the process was to get agreement on go-nuts before proposing a patch to golang. If that is not the case, let me know and I will propose a patch directly.

rjnagal · 2014-08-13T19:55:09Z

@mrunalp I think we might get more discussion after we propose the flag. No discussion usually means no one hates the suggestion :) But they might come back when there's existing code. I'll ping some go guys locally.

mrunalp · 2014-08-13T20:07:58Z

@rjnagal Ahh, well -- that's good to know :) I will start on the proposal. Let us know what the go folks think.

bradfitz · 2014-08-13T20:52:48Z

golang-nuts is the user list. You'd get more discussion on golang-dev. In this case I'd even say file a bug so we track it: https://code.google.com/p/go/issues/list

mrunalp · 2014-08-13T21:08:44Z

@bradfitz Thanks for chiming in! :) I did open an issue -- https://code.google.com/p/go/issues/detail?id=8447, but trying to figure out the next steps. I have posted to golang-dev just now -- https://groups.google.com/forum/#!topic/golang-dev/qZeWlFQ_hLw. Should I wait for a discussion or start the code review process as described here -- http://golang.org/doc/contribute.html ?

mrunalp · 2014-08-13T22:43:46Z

https://codereview.appspot.com/126190043 opened

crosbymichael · 2014-08-13T22:54:42Z

I'll close this for now until we can figure out if this should go in the Go stdlib

mrunalp · 2014-08-13T23:23:15Z

SGTM.

tianon reviewed Aug 8, 2014
View reviewed changes

crosbymichael reviewed Aug 8, 2014
View reviewed changes

mrunalp changed the title ~~Add forklib that supports user namespaces in Go.~~ Add forkexec library that supports user namespaces in Go. Aug 8, 2014

mrunalp changed the title ~~Add forkexec library that supports user namespaces in Go.~~ Adds forkexec library that supports user namespaces in Go. Aug 8, 2014

philips reviewed Aug 11, 2014
View reviewed changes

Add forkexec library that supports user namespaces in Go.

df2e7ef

Docker-DCO-1.1-Signed-off-by: Mrunal Patel <[email protected]> (github: mrunalp)

crosbymichael closed this Aug 13, 2014

bobtfish mentioned this pull request Aug 25, 2014

Use user namespaces Yelp/dockersh#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds forkexec library that supports user namespaces in Go. #150

Adds forkexec library that supports user namespaces in Go. #150

mrunalp commented Aug 8, 2014

mrunalp commented Aug 8, 2014

tianon Aug 8, 2014

mrunalp Aug 8, 2014

crosbymichael commented Aug 8, 2014

crosbymichael Aug 8, 2014

mrunalp Aug 8, 2014

crosbymichael commented Aug 8, 2014

mrunalp commented Aug 8, 2014

mrunalp commented Aug 8, 2014

philips Aug 11, 2014

mrunalp Aug 11, 2014

philips commented Aug 11, 2014

mrunalp commented Aug 12, 2014

philips commented Aug 12, 2014

vmarmol commented Aug 13, 2014

rjnagal commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014

philips commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

rjnagal commented Aug 13, 2014

mrunalp commented Aug 13, 2014

bradfitz commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014

Adds forkexec library that supports user namespaces in Go. #150

Adds forkexec library that supports user namespaces in Go. #150

Conversation

mrunalp commented Aug 8, 2014

mrunalp commented Aug 8, 2014

tianon Aug 8, 2014

Choose a reason for hiding this comment

mrunalp Aug 8, 2014

Choose a reason for hiding this comment

crosbymichael commented Aug 8, 2014

crosbymichael Aug 8, 2014

Choose a reason for hiding this comment

mrunalp Aug 8, 2014

Choose a reason for hiding this comment

crosbymichael commented Aug 8, 2014

mrunalp commented Aug 8, 2014

mrunalp commented Aug 8, 2014

philips Aug 11, 2014

Choose a reason for hiding this comment

mrunalp Aug 11, 2014

Choose a reason for hiding this comment

philips commented Aug 11, 2014

mrunalp commented Aug 12, 2014

philips commented Aug 12, 2014

vmarmol commented Aug 13, 2014

rjnagal commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014

philips commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

rjnagal commented Aug 13, 2014

mrunalp commented Aug 13, 2014

bradfitz commented Aug 13, 2014

mrunalp commented Aug 13, 2014

mrunalp commented Aug 13, 2014

crosbymichael commented Aug 13, 2014

mrunalp commented Aug 13, 2014