[libvirt] [PATCH] lxc: hoist supplemental group detection before clone

[Daniel P. Berrange]

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 at 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 :|