[libvirt] [PATCH] lxc: hoist supplemental group detection before clone
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc. Hoist the group detection to occur before clone. * src/lxc/lxc_container.c (__lxc_child_argv): Add members. (lxcContainerSetID): Adjust signature. (lxcContainerChild, lxcContainerStart): Adjust callers. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_container.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 940233b..50aaa36 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -106,6 +106,8 @@ struct __lxc_child_argv { char **ttyPaths; size_t nttyPaths; int handshakefd; + gid_t *groups; + int ngroups; }; static int lxcContainerMountFSBlock(virDomainFSDefPtr fs, @@ -349,26 +351,20 @@ int lxcContainerWaitForContinue(int control) * * Returns 0 on success or -1 in case of error */ -static int lxcContainerSetID(virDomainDefPtr def) +static int +lxcContainerSetID(virDomainDefPtr def, gid_t* groups, int ngroups) { - gid_t *groups; - int ngroups; - /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */ VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && - ((ngroups = virGetGroupList(0, 0, &groups) < 0) || - virSetUIDGID(0, 0, groups, ngroups) < 0)) { + if (def->idmap.nuidmap && virSetUIDGID(0, 0, groups, ngroups) < 0) { virReportSystemError(errno, "%s", _("setuid or setgid failed")); - VIR_FREE(groups); return -1; } - VIR_FREE(groups); return 0; } @@ -1981,7 +1977,7 @@ static int lxcContainerChild(void *data) cmd = lxcContainerBuildInitCmd(vmDef); virCommandWriteArgLog(cmd, 1); - if (lxcContainerSetID(vmDef) < 0) + if (lxcContainerSetID(vmDef, argv->groups, argv->ngroups) < 0) goto cleanup; root = virDomainGetRootFilesystem(vmDef); @@ -2054,6 +2050,7 @@ cleanup: VIR_FORCE_CLOSE(ttyfd); VIR_FORCE_CLOSE(argv->monitor); VIR_FORCE_CLOSE(argv->handshakefd); + VIR_FREE(argv->groups); if (ret == 0) { /* this function will only return if an error occurred */ @@ -2140,13 +2137,18 @@ int lxcContainerStart(virDomainDefPtr def, char *stack, *stacktop; lxc_child_argv_t args = { def, securityDriver, nveths, veths, control, - ttyPaths, nttyPaths, handshakefd}; + ttyPaths, nttyPaths, handshakefd, NULL, 0 }; /* allocate a stack for the container */ if (VIR_ALLOC_N(stack, stacksize) < 0) return -1; stacktop = stack + stacksize; + if ((args.ngroups = virGetGroupList(0, 0, &args.groups)) < 0) { + VIR_FREE(stack); + return -1; + } + cflags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD; if (userns_required(def)) { @@ -2157,6 +2159,7 @@ int lxcContainerStart(virDomainDefPtr def, virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Kernel doesn't support user namespace")); VIR_FREE(stack); + VIR_FREE(args.groups); return -1; } } @@ -2168,6 +2171,7 @@ int lxcContainerStart(virDomainDefPtr def, pid = clone(lxcContainerChild, stacktop, cflags, &args); VIR_FREE(stack); + VIR_FREE(args.groups); VIR_DEBUG("clone() completed, new container PID is %d", pid); if (pid < 0) { -- 1.8.1.4
On 07/13/2013 01:35 AM, Eric Blake wrote:
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc. Hoist the group detection to occur before clone.
* src/lxc/lxc_container.c (__lxc_child_argv): Add members. (lxcContainerSetID): Adjust signature. (lxcContainerChild, lxcContainerStart): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_container.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 940233b..50aaa36 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -106,6 +106,8 @@ struct __lxc_child_argv { char **ttyPaths; size_t nttyPaths; int handshakefd; + gid_t *groups; + int ngroups; };
static int lxcContainerMountFSBlock(virDomainFSDefPtr fs, @@ -349,26 +351,20 @@ int lxcContainerWaitForContinue(int control) * * Returns 0 on success or -1 in case of error */ -static int lxcContainerSetID(virDomainDefPtr def) +static int +lxcContainerSetID(virDomainDefPtr def, gid_t* groups, int ngroups) { - gid_t *groups; - int ngroups; - /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */
VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && - ((ngroups = virGetGroupList(0, 0, &groups) < 0) ||
this is a bug... ngroups is 0 here if virGetGroupList(,,) is greater than 0.. I personal think it has no bad affection if we don't set supplementary groups here. Since we are not quite care about what the supplementary groups the root user of container belongs to. and if we want to set supplementary groups for the users of container, we should also make sure these supplementary groups is valid in the user namespace this container belongs to. setgroups will failed if we don't have proper <idmap><gid/></idmap> context. Thanks
On Mon, Jul 15, 2013 at 03:40:53PM +0800, Gao feng wrote:
On 07/13/2013 01:35 AM, Eric Blake wrote:
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc. Hoist the group detection to occur before clone.
* src/lxc/lxc_container.c (__lxc_child_argv): Add members. (lxcContainerSetID): Adjust signature. (lxcContainerChild, lxcContainerStart): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_container.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 940233b..50aaa36 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -106,6 +106,8 @@ struct __lxc_child_argv { char **ttyPaths; size_t nttyPaths; int handshakefd; + gid_t *groups; + int ngroups; };
static int lxcContainerMountFSBlock(virDomainFSDefPtr fs, @@ -349,26 +351,20 @@ int lxcContainerWaitForContinue(int control) * * Returns 0 on success or -1 in case of error */ -static int lxcContainerSetID(virDomainDefPtr def) +static int +lxcContainerSetID(virDomainDefPtr def, gid_t* groups, int ngroups) { - gid_t *groups; - int ngroups; - /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */
VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && - ((ngroups = virGetGroupList(0, 0, &groups) < 0) ||
this is a bug... ngroups is 0 here if virGetGroupList(,,) is greater than 0..
I personal think it has no bad affection if we don't set supplementary groups here.
Yeah, I'm inclined to say that we should not set any supplementary groups here.
Since we are not quite care about what the supplementary groups the root user of container belongs to. and if we want to set supplementary groups for the users of container, we should also make sure these supplementary groups is valid in the user namespace this container belongs to. setgroups will failed if we don't have proper <idmap><gid/></idmap> context.
'initgroups' will continually call setgroups() removing the last GID from the list each time until it succeeds. This is somewhat dubious behaviour IMHO, which again makes me think we should just not set any supplementary groups in LXC Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/15/2013 03:59 AM, Daniel P. Berrange wrote:
On Mon, Jul 15, 2013 at 03:40:53PM +0800, Gao feng wrote:
On 07/13/2013 01:35 AM, Eric Blake wrote:
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc. Hoist the group detection to occur before clone.
* src/lxc/lxc_container.c (__lxc_child_argv): Add members. (lxcContainerSetID): Adjust signature. (lxcContainerChild, lxcContainerStart): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
- /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */
VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && - ((ngroups = virGetGroupList(0, 0, &groups) < 0) ||
'initgroups' will continually call setgroups() removing the last GID from the list each time until it succeeds. This is somewhat dubious behaviour IMHO, which again makes me think we should just not set any supplementary groups in LXC
Then it sounds like I need a v2 that calls virSetUIDGID(0, 0, NULL, 0), which forces the implementation to not set any supplementary groups. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/16/2013 04:23 AM, Eric Blake wrote:
On 07/15/2013 03:59 AM, Daniel P. Berrange wrote:
On Mon, Jul 15, 2013 at 03:40:53PM +0800, Gao feng wrote:
On 07/13/2013 01:35 AM, Eric Blake wrote:
Commit 75c1256 states that virGetGroupList must not be called between fork and exec, then commit ee777e99 promptly violated that for lxc. Hoist the group detection to occur before clone.
* src/lxc/lxc_container.c (__lxc_child_argv): Add members. (lxcContainerSetID): Adjust signature. (lxcContainerChild, lxcContainerStart): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
- /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */
VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && - ((ngroups = virGetGroupList(0, 0, &groups) < 0) ||
'initgroups' will continually call setgroups() removing the last GID from the list each time until it succeeds. This is somewhat dubious behaviour IMHO, which again makes me think we should just not set any supplementary groups in LXC
Then it sounds like I need a v2 that calls virSetUIDGID(0, 0, NULL, 0), which forces the implementation to not set any supplementary groups.
Yeah, Please :) Thanks
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Gao feng