|
|
Descriptionsyscall: Adds support for User Namespaces in Linux by allowing to specify UID/GID
mappings in syscall.Credential.
Fixes issue 8447.
Patch Set 1 #Patch Set 2 : diff -r 2cc71cd8fe3e466a2338b1fdf8053931d47a9e31 https://code.google.com/p/go #
Total comments: 20
Patch Set 3 : diff -r 1c674c3eefc7c3c2c73378bd2482a08b933839ee https://code.google.com/p/go #
Total comments: 16
Patch Set 4 : diff -r 1c674c3eefc7c3c2c73378bd2482a08b933839ee https://code.google.com/p/go #
Total comments: 3
Patch Set 5 : diff -r ce32e953ef6f5418efbcce5c25175aec25204eac https://code.google.com/p/go #
Total comments: 17
Patch Set 6 : diff -r ce32e953ef6f5418efbcce5c25175aec25204eac https://code.google.com/p/go #
Total comments: 4
Patch Set 7 : diff -r ce32e953ef6f5418efbcce5c25175aec25204eac https://code.google.com/p/go #
Total comments: 2
Patch Set 8 : diff -r ce32e953ef6f5418efbcce5c25175aec25204eac https://code.google.com/p/go #MessagesTotal messages: 25
The code adds support for User Namespaces to golang. For issues and reason behind why it is being done in fork/exec code see https://code.google.com/p/go/issues/detail?id=8447 - Mrunal
Sign in to reply to this message.
R=iant On Wed, Sep 24, 2014 at 2:49 PM, <mrunalp@gmail.com> wrote: > Reviewers: golang-codereviews, > > Message: > The code adds support for User Namespaces to golang. > For issues and reason behind why it is being done in fork/exec code see > https://code.google.com/p/go/issues/detail?id=8447 > > - Mrunal > > > Description: > syscall: Adds support for User Namespaces in Linux by allowing to > specify UID/GID > mappings in syscall.Credential. > Fixes issue 8447. > > Please review this at https://codereview.appspot.com/126190043/ > > Affected files (+69, -11 lines): > M src/pkg/syscall/exec_linux.go > M src/pkg/syscall/exec_unix.go > > > Index: src/pkg/syscall/exec_linux.go > =================================================================== > --- a/src/pkg/syscall/exec_linux.go > +++ b/src/pkg/syscall/exec_linux.go > @@ -36,7 +36,7 @@ > // For the same reason compiler does not race instrument it. > // The calls to RawSyscall are okay because they are assembly > // functions that do not grow the stack. > -func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir > *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (pid int, err Errno) { > +func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir > *byte, attr *ProcAttr, sys *SysProcAttr, child, parent int) (pid int, err > Errno) { > // Declare all variables at top in case any > // declarations require heap allocation (e.g., err1). > var ( > @@ -44,6 +44,7 @@ > err1 Errno > nextfd int > i int > + lzero uintptr > ) > > // Guard against side effects of shuffling fds below. > @@ -75,6 +76,16 @@ > } > > // Fork succeeded, now in child. > + if _, _, err1 = RawSyscall(SYS_CLOSE, uintptr(child), 0, 0); err > != 0 { > + goto childerror > + } > + > + if sys.Credential != nil && (sys.Credential.UidMappings != "" || > sys.Credential.GidMappings != "") { > + _, _, err1 = RawSyscall(SYS_READ, uintptr(parent), > uintptr(unsafe.Pointer(&lzero)), uintptr(1)) > + if err1 != 0 { > + goto childerror > + } > + } > > // Parent death signal > if sys.Pdeathsig != 0 { > @@ -159,13 +170,13 @@ > > // Pass 1: look for fd[i] < i and move those up above len(fd) > // so that pass 2 won't stomp on an fd it needs later. > - if pipe < nextfd { > - _, _, err1 = RawSyscall(SYS_DUP2, uintptr(pipe), > uintptr(nextfd), 0) > + if child < nextfd { > + _, _, err1 = RawSyscall(SYS_DUP2, uintptr(child), > uintptr(nextfd), 0) > if err1 != 0 { > goto childerror > } > RawSyscall(SYS_FCNTL, uintptr(nextfd), F_SETFD, FD_CLOEXEC) > - pipe = nextfd > + child = nextfd > nextfd++ > } > for i = 0; i < len(fd); i++ { > @@ -177,7 +188,7 @@ > RawSyscall(SYS_FCNTL, uintptr(nextfd), F_SETFD, > FD_CLOEXEC) > fd[i] = nextfd > nextfd++ > - if nextfd == pipe { // don't stomp on pipe > + if nextfd == child { // don't stomp on pipe > nextfd++ > } > } > @@ -237,8 +248,8 @@ > uintptr(unsafe.Pointer(&envv[0]))) > > childerror: > - // send error code on pipe > - RawSyscall(SYS_WRITE, uintptr(pipe), uintptr(unsafe.Pointer(&err1)), > unsafe.Sizeof(err1)) > + // send error code on child > + RawSyscall(SYS_WRITE, uintptr(child), > uintptr(unsafe.Pointer(&err1)), unsafe.Sizeof(err1)) > for { > RawSyscall(SYS_EXIT, 253, 0, 0) > } > Index: src/pkg/syscall/exec_unix.go > =================================================================== > --- a/src/pkg/syscall/exec_unix.go > +++ b/src/pkg/syscall/exec_unix.go > @@ -110,9 +110,11 @@ > // Credential holds user and group identities to be assumed > // by a child process started by StartProcess. > type Credential struct { > - Uid uint32 // User ID. > - Gid uint32 // Group ID. > - Groups []uint32 // Supplementary group IDs. > + Uid uint32 // User ID. > + Gid uint32 // Group ID. > + Groups []uint32 // Supplementary group IDs. > + UidMappings string > + GidMappings string > } > > // ProcAttr holds attributes that will be applied to a new process started > @@ -188,13 +190,19 @@ > } > > // Kick off child. > - pid, err1 = forkAndExecInChild(argv0p, argvp, envvp, chroot, dir, > attr, sys, p[1]) > + pid, err1 = forkAndExecInChild(argv0p, argvp, envvp, chroot, dir, > attr, sys, p[1], p[0]) > if err1 != 0 { > err = Errno(err1) > goto error > } > ForkLock.Unlock() > > + if sys.Credential != nil { > + if err = writeUidGidMappings(pid, sys.Credential); err != > nil { > + goto error > + } > + } > + > // Read child error status from pipe. > Close(p[1]) > n, err = readlen(p[0], (*byte)(unsafe.Pointer(&err1)), > int(unsafe.Sizeof(err1))) > @@ -259,3 +267,42 @@ > uintptr(unsafe.Pointer(&envvp[0]))) > return Errno(err1) > } > + > +func writeUidGidMappings(pid int, cred *Credential) error { > + var ( > + uidf = "/proc/" + itoa(pid) + "/uid_map" > + gidf = "/proc/" + itoa(pid) + "/gid_map" > + ) > + > + if cred.UidMappings != "" { > + fd, err := Open(uidf, O_RDWR, 0) > + if err != nil { > + return err > + } > + > + data := StringByteSlice(cred.UidMappings) > + if _, err := Write(fd, data); err != nil { > + Close(fd) > + return err > + } > + > + Close(fd) > + } > + > + if cred.GidMappings != "" { > + fd, err := Open(gidf, O_RDWR, 0) > + if err != nil { > + return err > + } > + > + data := StringByteSlice(cred.GidMappings) > + if _, err := Write(fd, data); err != nil { > + Close(fd) > + return err > + } > + > + Close(fd) > + } > + > + return nil > +} > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >
Sign in to reply to this message.
The first line in the CL description should be a single line with a description that is not a full sentence--no initial capital, no period. For example: syscall: support for UID/GID mappings for Linux https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... File src/pkg/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... src/pkg/syscall/exec_linux.go:75: return int(r1), 0 This code has changed, you need to sync with tip. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... src/pkg/syscall/exec_linux.go:84: _, _, err1 = RawSyscall(SYS_READ, uintptr(parent), uintptr(unsafe.Pointer(&lzero)), uintptr(1)) Where is this byte written? https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix.go File src/pkg/syscall/exec_unix.go (right): https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:116: UidMappings string If I understand this code correctly, these mappings are GNU/Linux specific. I'm not sure they should be in this generic struct. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:116: UidMappings string These fields need documentation. If I understand this code correctly, the formatting of these fields is quite important. Looking at the docs, it appears that the fields must be newline separated lines where each line has exactly three numbers. If that is the case, then perhaps these fields should have type [][3]int. Making these string seems designed to let people use them incorrectly. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:193: pid, err1 = forkAndExecInChild(argv0p, argvp, envvp, chroot, dir, attr, sys, p[1], p[0]) This will break every Unix/non-Linux build. There are three other instances of forkAndExecInChild that will have to change. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:201: if err = writeUidGidMappings(pid, sys.Credential); err != nil { Since all this code appears to be GNU/Linux-specific, why not do it in exec_linux.go? https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:273: uidf = "/proc/" + itoa(pid) + "/uid_map" Move these settings down to where they are needed. Use :=. if cred.UidMappings != "" { uidf := "/proc/" + itoa(pid) + "/uid_map" ... https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:283: data := StringByteSlice(cred.UidMappings) As the comment says, StringByteSlice is deprecated. Use ByteSliceFromString. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:284: if _, err := Write(fd, data); err != nil { This will write a trailing NUL byte. Is that correct? https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:289: Close(fd) Always check errors when closing a file opened for writing.
Sign in to reply to this message.
I am working on addressing the comments. Meanwhile there is one design decision to be made about how to pass the mappings down if not in Credential. Thanks, Mrunal https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... File src/pkg/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... src/pkg/syscall/exec_linux.go:75: return int(r1), 0 On 2014/09/30 14:14:16, iant wrote: > This code has changed, you need to sync with tip. Acknowledged. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_linu... src/pkg/syscall/exec_linux.go:84: _, _, err1 = RawSyscall(SYS_READ, uintptr(parent), uintptr(unsafe.Pointer(&lzero)), uintptr(1)) Closing the other end of the pipe causes a NUL byte to be written. On 2014/09/30 14:14:17, iant wrote: > Where is this byte written? https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix.go File src/pkg/syscall/exec_unix.go (right): https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:116: UidMappings string We can introduce a IdMap struct with 3 fields type IdMap struct { ContainerId uint32 HostId uint32 Size uint32 } UidMappings []IdMap GidMappings []IdMap Question is how do we pass them down if not in the Credential struct? On 2014/09/30 14:14:17, iant wrote: > If I understand this code correctly, these mappings are GNU/Linux specific. I'm > not sure they should be in this generic struct. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:116: UidMappings string Adding documentation. See comment for next line. On 2014/09/30 14:14:17, iant wrote: > These fields need documentation. If I understand this code correctly, the > formatting of these fields is quite important. > > Looking at the docs, it appears that the fields must be newline separated lines > where each line has exactly three numbers. If that is the case, then perhaps > these fields should have type [][3]int. Making these string seems designed to > let people use them incorrectly. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:193: pid, err1 = forkAndExecInChild(argv0p, argvp, envvp, chroot, dir, attr, sys, p[1], p[0]) On 2014/09/30 14:14:17, iant wrote: > This will break every Unix/non-Linux build. There are three other instances of > forkAndExecInChild that will have to change. Acknowledged. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:201: if err = writeUidGidMappings(pid, sys.Credential); err != nil { Looking into it. On 2014/09/30 14:14:17, iant wrote: > Since all this code appears to be GNU/Linux-specific, why not do it in > exec_linux.go? https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:273: uidf = "/proc/" + itoa(pid) + "/uid_map" On 2014/09/30 14:14:17, iant wrote: > Move these settings down to where they are needed. Use :=. > if cred.UidMappings != "" { > uidf := "/proc/" + itoa(pid) + "/uid_map" > ... Acknowledged. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:283: data := StringByteSlice(cred.UidMappings) On 2014/09/30 14:14:17, iant wrote: > As the comment says, StringByteSlice is deprecated. Use ByteSliceFromString. Acknowledged. https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:289: Close(fd) On 2014/09/30 14:14:17, iant wrote: > Always check errors when closing a file opened for writing. Acknowledged.
Sign in to reply to this message.
https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix.go File src/pkg/syscall/exec_unix.go (right): https://codereview.appspot.com/126190043/diff/20001/src/pkg/syscall/exec_unix... src/pkg/syscall/exec_unix.go:116: UidMappings string On 2014/09/30 18:42:06, mrunalp wrote: > We can introduce a IdMap struct with 3 fields > > type IdMap struct { > ContainerId uint32 > HostId uint32 > Size uint32 > } I would make all of these type "int" unless there is some reason to do otherwise. Are there systems with UIDs or GIDs > 0x7fffffff? > UidMappings []IdMap > GidMappings []IdMap > > Question is how do we pass them down if not in the Credential struct? Add them to the GNU/Linux-specific version of SysProcAttr, defined in syscall/exec_linux.go.
Sign in to reply to this message.
> > We can introduce a IdMap struct with 3 fields > > > > type IdMap struct { > > ContainerId uint32 > > HostId uint32 > > Size uint32 > > } > > > I would make all of these type "int" unless there is some reason to do > otherwise. Are there systems with UIDs or GIDs > 0x7fffffff? > It seems to be stored as uint32 in the kernel -- http://lxr.free-electrons.com/source/include/linux/user_namespace.h#L11 Also, in glibc -- [root@dhcp-16-139 ~]$ grep '#define __UID_T_TYPE' /usr/include/bits/typesizes.h #define __UID_T_TYPE __U32_TYPE > > > > UidMappings []IdMap > > GidMappings []IdMap > > > > Question is how do we pass them down if not in the Credential struct? > > Add them to the GNU/Linux-specific version of SysProcAttr, defined in > syscall/exec_linux.go. Sure, I can do that.
Sign in to reply to this message.
On Tue, Sep 30, 2014 at 12:22 PM, <mrunalp@gmail.com> wrote: >> > We can introduce a IdMap struct with 3 fields >> > >> > type IdMap struct { >> > ContainerId uint32 >> > HostId uint32 >> > Size uint32 >> > } > > > >> I would make all of these type "int" unless there is some reason to do >> otherwise. Are there systems with UIDs or GIDs > 0x7fffffff? > > > > It seems to be stored as uint32 in the kernel -- > http://lxr.free-electrons.com/source/include/linux/user_namespace.h#L11 > > Also, in glibc -- > [root@dhcp-16-139 ~]$ grep '#define __UID_T_TYPE' > /usr/include/bits/typesizes.h > #define __UID_T_TYPE __U32_TYPE Understood, but not wholly relevant. We want the right API for Go, which need not be the exact type used in the kernel. In Go syscall.Getuid returns a value of type int, not a value of type Uid_t. We should use int here unless there an explicit reason not to do so. Ian
Sign in to reply to this message.
> > Understood, but not wholly relevant. We want the right API for Go, > which need not be the exact type used in the kernel. In Go > syscall.Getuid returns a value of type int, not a value of type Uid_t. > We should use int here unless there an explicit reason not to do so. Sounds good to me.
Sign in to reply to this message.
I have updated the code to introduce IdMap struct and pushed down the functionality into exec_linux.go. If we create another pipe in forkAndExecInChild in exec_linux.go, then we can possibly avoid modifying the signature of the function. Any thoughts? Also, I have added a couple of inline comments that I would like feedback on. - Mrunal https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:90: return 0, err.(Errno) I am not sure if this is the right way to handle this. Also, what do we do in exec_unix if writing the mappings fail? Just goto error like now or attempt to kill the child process? https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:345: Close(fd) Not yet handles the errors from Close. I see similar pattern in exec_unix.go. I can add error codes in the next update anyways.
Sign in to reply to this message.
drive-by https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:13: // Holds Container to Host ID mappings used for User Namespaces in Linux. Style issue. See https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Comment_Sentences
Sign in to reply to this message.
Addressed stylistic issue (hopefully) and fixed fd leak.
Sign in to reply to this message.
I don't think there is anything wrong with changing the signature of the function, as long as the other instances are updated. I wouldn't add another pipe unless there is some benefit to that. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:58: lzero uintptr This can just be byte. Or you can just use err1. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:90: return 0, err.(Errno) On 2014/10/01 01:37:12, mrunalp wrote: > I am not sure if this is the right way to handle this. Also, what do we do in > exec_unix if writing the mappings fail? Just goto error like now or attempt to > kill the child process? Good point. If there are mappings, the child process is going to be waiting for a notification that it can continue. If there is an error writing the notifications, I think what we should do is use that notification to tell the child to exit. Probably we should pass the Errno value to the child, and then have it pass the same Errno value back to the parent process. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:326: func writeUidGidMappings(pid int, sys *SysProcAttr) error { Add a comment saying what this function does and noting that it is called in the parent process. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:335: for _, um := range sys.UidMappings { Might as well use a function to write out the mappings rather than repeating the code. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:341: return err Close(fd). https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:345: Close(fd) On 2014/10/01 01:37:12, mrunalp wrote: > Not yet handles the errors from Close. I see similar pattern in exec_unix.go. I > can add error codes in the next update anyways. The Close calls in exec_unix.go are for descriptors opened for reading, not writing. It's OK to ignore the error from closing a descriptor opened for reading, because there is nothing useful to do with the error. It's not OK to ignore the error from a descriptor opened for writing, because some errors on writing the data only appear when the descriptor is closed. https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:13: // IdMap holds Container ID to Host ID mappings used for User Namespaces in Linux. This is sufficiently obscure that I think it would help to add a link to some documentation. A reference to a man page would be fine if there is a man page for this. https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_unix.go File src/syscall/exec_unix.go (right): https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_unix.go#... src/syscall/exec_unix.go:196: No need to add this blank line.
Sign in to reply to this message.
I have addressed the comments and reworked the code a bit. Now, I am using a new pipe within forkAndExecInChild when there are uid/gid mappings passed as options. There was one issue in the previous version where we had to close the child end in order to receive the NUL on parent close. This prevented the child to send back errors to the parent. Creating this new pipe for parent to child synchronization solves that issue. Also, some reading suggests that it is better to use different pipes for each direction of communication. I still have a couple of inline questions. The big one being about what to do if parent process fails to write to the pipe to notify the child process. Thanks, Mrunal https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:13: // Holds Container to Host ID mappings used for User Namespaces in Linux. On 2014/10/01 03:40:07, bradfitz wrote: > Style issue. See > https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Comment_Sentences Done. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:58: lzero uintptr On 2014/10/01 14:55:08, iant wrote: > This can just be byte. Or you can just use err1. Done. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:326: func writeUidGidMappings(pid int, sys *SysProcAttr) error { On 2014/10/01 14:55:08, iant wrote: > Add a comment saying what this function does and noting that it is called in the > parent process. Done. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:335: for _, um := range sys.UidMappings { On 2014/10/01 14:55:08, iant wrote: > Might as well use a function to write out the mappings rather than repeating the > code. Done. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:341: return err On 2014/10/01 14:55:07, iant wrote: > Close(fd). Done. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:345: Close(fd) On 2014/10/01 14:55:08, iant wrote: > On 2014/10/01 01:37:12, mrunalp wrote: > > Not yet handles the errors from Close. I see similar pattern in exec_unix.go. > I > > can add error codes in the next update anyways. > > The Close calls in exec_unix.go are for descriptors opened for reading, not > writing. It's OK to ignore the error from closing a descriptor opened for > reading, because there is nothing useful to do with the error. It's not OK to > ignore the error from a descriptor opened for writing, because some errors on > writing the data only appear when the descriptor is closed. Acknowledged. https://codereview.appspot.com/126190043/diff/40001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:345: Close(fd) On 2014/10/01 14:55:08, iant wrote: > On 2014/10/01 01:37:12, mrunalp wrote: > > Not yet handles the errors from Close. I see similar pattern in exec_unix.go. > I > > can add error codes in the next update anyways. > > The Close calls in exec_unix.go are for descriptors opened for reading, not > writing. It's OK to ignore the error from closing a descriptor opened for > reading, because there is nothing useful to do with the error. It's not OK to > ignore the error from a descriptor opened for writing, because some errors on > writing the data only appear when the descriptor is closed. Done. https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_unix.go File src/syscall/exec_unix.go (right): https://codereview.appspot.com/126190043/diff/60001/src/syscall/exec_unix.go#... src/syscall/exec_unix.go:196: On 2014/10/01 14:55:08, iant wrote: > No need to add this blank line. Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:14: type IdMap struct { Should I rename Id --> ID everywhere as per naming guidelines? https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:105: RawSyscall(SYS_WRITE, uintptr(p[1]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) Again questions around error-handling here. There isn't much we can do if we can't send this to the child. Does a panic work or make sense here?
Sign in to reply to this message.
Getting close, I think. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:13: // IdMap holds Container ID to Host ID mappings used for User Namespaces in Linux. See user_namespaces(7). Please break the comment line after "Linux.". https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:14: type IdMap struct { On 2014/10/01 21:48:42, mrunalp wrote: > Should I rename Id --> ID everywhere as per naming guidelines? Yes, please, sorry I missed that. Actually IDMap is not a great name, really, not that there are any great names. This is so specific to GNU/Linux and to Exec. Maybe SysProcIDMap would convey the right association with SysProcAttr. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:76: p[0] = -1 No need to set p[0] and p[1] to -1. I'm not sure why the code in exec_unix.go does that. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:99: var err2 uintptr Should probably declare err2 up above next to err1. No need to set err2 to 0, it's the default value anyhow. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:105: RawSyscall(SYS_WRITE, uintptr(p[1]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) On 2014/10/01 21:48:42, mrunalp wrote: > Again questions around error-handling here. There isn't much we can do if we > can't send this to the child. Does a panic work or make sense here? Fortunately I don't think we have to worry about this (somewhat unlikely) error case. It's hard to see how closing the pipe could fail, and that will mean that the child will fail to read the expected value, so the child will wind up exiting anyhow. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:132: _, _, err2 := RawSyscall(SYS_READ, uintptr(p[0]), uintptr(unsafe.Pointer(&err1)), uintptr(1)) The parent is going to write unsafe.Sizeof(uintptr(0)) bytes, and that is how many we should read here. We shouldn't pass &err1, we should pass &err2. We should err1 here, and not use :=. We should catch the first return value, which is the number of bytes read, and make sure it is == unsafe.Sizeof(uintptr(0)). https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:376: // writeUidGidMappings writes uid/gid mappings for user namespaces for a process and it is called from the parent process. Add a line break somewhere in the comment.
Sign in to reply to this message.
Thanks for the review comments Ian! I have addressed all the comments (I think). I only changed instances of "Id" to "ID". Should I be more aggressive and change UidMappings to UserIDMappings, etc.? Also, one inline question about setting err1 when we don't get the expected number of bytes back from the read system call. Thanks, Mrunal https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:13: // IdMap holds Container ID to Host ID mappings used for User Namespaces in Linux. See user_namespaces(7). On 2014/10/01 22:27:18, iant wrote: > Please break the comment line after "Linux.". Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:14: type IdMap struct { On 2014/10/01 22:27:18, iant wrote: > On 2014/10/01 21:48:42, mrunalp wrote: > > Should I rename Id --> ID everywhere as per naming guidelines? > > Yes, please, sorry I missed that. > > Actually IDMap is not a great name, really, not that there are any great names. > This is so specific to GNU/Linux and to Exec. Maybe SysProcIDMap would convey > the right association with SysProcAttr. Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:76: p[0] = -1 On 2014/10/01 22:27:18, iant wrote: > No need to set p[0] and p[1] to -1. I'm not sure why the code in exec_unix.go > does that. Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:99: var err2 uintptr On 2014/10/01 22:27:18, iant wrote: > Should probably declare err2 up above next to err1. No need to set err2 to 0, > it's the default value anyhow. Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:105: RawSyscall(SYS_WRITE, uintptr(p[1]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) On 2014/10/01 22:27:18, iant wrote: > On 2014/10/01 21:48:42, mrunalp wrote: > > Again questions around error-handling here. There isn't much we can do if we > > can't send this to the child. Does a panic work or make sense here? > > Fortunately I don't think we have to worry about this (somewhat unlikely) error > case. It's hard to see how closing the pipe could fail, and that will mean that > the child will fail to read the expected value, so the child will wind up > exiting anyhow. Acknowledged. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:132: _, _, err2 := RawSyscall(SYS_READ, uintptr(p[0]), uintptr(unsafe.Pointer(&err1)), uintptr(1)) Ahh, I missed that. Thanks! https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:132: _, _, err2 := RawSyscall(SYS_READ, uintptr(p[0]), uintptr(unsafe.Pointer(&err1)), uintptr(1)) On 2014/10/01 22:27:17, iant wrote: > The parent is going to write unsafe.Sizeof(uintptr(0)) bytes, and that is how > many we should read here. We shouldn't pass &err1, we should pass &err2. We > should err1 here, and not use :=. We should catch the first return value, which > is the number of bytes read, and make sure it is == unsafe.Sizeof(uintptr(0)). Done. https://codereview.appspot.com/126190043/diff/80001/src/syscall/exec_linux.go... src/syscall/exec_linux.go:376: // writeUidGidMappings writes uid/gid mappings for user namespaces for a process and it is called from the parent process. On 2014/10/01 22:27:18, iant wrote: > Add a line break somewhere in the comment. Done. https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:132: if err1 != 0 || r1 != unsafe.Sizeof(uintptr(0)) { Should we set err1 to something specific when r1 doesn't have the expected number of bytes?
Sign in to reply to this message.
https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:132: if err1 != 0 || r1 != unsafe.Sizeof(uintptr(0)) { On 2014/10/02 00:07:04, mrunalp wrote: > Should we set err1 to something specific when r1 doesn't have the expected > number of bytes? Yes. I guess EINVAL unless you can think of something better.
Sign in to reply to this message.
I added EINVAL for the case described earlier. Thanks, Mrunal https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:132: if err1 != 0 || r1 != unsafe.Sizeof(uintptr(0)) { On 2014/10/02 00:33:34, iant wrote: > On 2014/10/02 00:07:04, mrunalp wrote: > > Should we set err1 to something specific when r1 doesn't have the expected > > number of bytes? > > Yes. I guess EINVAL unless you can think of something better. Sounds good to me. https://codereview.appspot.com/126190043/diff/100001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:132: if err1 != 0 || r1 != unsafe.Sizeof(uintptr(0)) { On 2014/10/02 00:33:34, iant wrote: > On 2014/10/02 00:07:04, mrunalp wrote: > > Should we set err1 to something specific when r1 doesn't have the expected > > number of bytes? > > Yes. I guess EINVAL unless you can think of something better. Done.
Sign in to reply to this message.
Have you signed the CLA as described at http://golang.org/doc/contribute.html ?
Sign in to reply to this message.
https://codereview.appspot.com/126190043/diff/120001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/120001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:135: if r1 != unsafe.Sizeof(uintptr(0)) { Let's make this unsafe.Sizeof(err2), since that is what the SYS_READ call uses.
Sign in to reply to this message.
I made the suggested change. Also, I think there is a corporate CLA between Google and Red Hat. Other Red Hat folks haven't have had to sign the CLA while contributing to kubernetes. I think that I might be covered under the same CLA. I will check with those who might know and reply back. Thanks, Mrunal https://codereview.appspot.com/126190043/diff/120001/src/syscall/exec_linux.go File src/syscall/exec_linux.go (right): https://codereview.appspot.com/126190043/diff/120001/src/syscall/exec_linux.g... src/syscall/exec_linux.go:135: if r1 != unsafe.Sizeof(uintptr(0)) { On 2014/10/02 15:53:45, iant wrote: > Let's make this unsafe.Sizeof(err2), since that is what the SYS_READ call uses. Done.
Sign in to reply to this message.
On Thu, Oct 2, 2014 at 10:01 AM, <mrunalp@gmail.com> wrote: > > Also, I think there is a corporate CLA between Google and Red Hat. Other > Red Hat folks haven't have had to sign the CLA while contributing to > kubernetes. I think that I might be covered under the same CLA. I will > check with those who might know and reply back. You're right, there is. Thanks. Ian
Sign in to reply to this message.
LGTM Thanks for your patience with this.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=d56326309b20 *** syscall: support UID/GID map files for Linux user namespaces Fixes issue 8447. LGTM=iant R=golang-codereviews, bradfitz, iant CC=golang-codereviews https://codereview.appspot.com/126190043 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
On 2014/10/02 18:37:10, iant wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=d56326309b20 *** > > syscall: support UID/GID map files for Linux user namespaces > > Fixes issue 8447. > > LGTM=iant > R=golang-codereviews, bradfitz, iant > CC=golang-codereviews > https://codereview.appspot.com/126190043 > > Committer: Ian Lance Taylor <mailto:iant@golang.org> Thanks Ian! Is this making into go 1.4? - Mrunal
Sign in to reply to this message.
On 2014/10/02 19:36:06, mrunalp wrote: > On 2014/10/02 18:37:10, iant wrote: > > *** Submitted as https://code.google.com/p/go/source/detail?r=d56326309b20 *** > > > > syscall: support UID/GID map files for Linux user namespaces > > > > Fixes issue 8447. > > > > LGTM=iant > > R=golang-codereviews, bradfitz, iant > > CC=golang-codereviews > > https://codereview.appspot.com/126190043 > > > > Committer: Ian Lance Taylor <mailto:iant@golang.org> > > Thanks Ian! > Is this making into go 1.4? > > - Mrunal Never mind. I saw your response on the issue. Thanks again!
Sign in to reply to this message.
|