Re: [PATCH] LXC: do setxid before lxc controller creates fuse thread

["Daniel P. Berrange" <berrange at redhat dot com>]

On Thu, Nov 14, 2013 at 11:35:06AM +0800, Gao feng wrote:
> I met a problem that container blocked by seteuid/setegid
> which is call in lxcContainerSetID on UP system and libvirt
> compiled with --with-fuse=yes.
> 
> I looked into the glibc's codes, and found setxid in glibc
> calls futex() to wait for other threads to change their
> setxid_futex to 0(see setxid_mark_thread in glibc).
> 
> since the process created by clone system call will not
> share the memory with the other threads and the context
> of memory doesn't changed until we call execl.(COW)
> 
> So if the process which created by clone is called before
> fuse thread being stated, the new setxid_futex of fuse
> thread will not be saw in this process, it will be blocked
> forever.
> 
> This patch makes sure the cloned process calls setxid first,
> and then the lxc controller creates fuse thread.

This is not required. All we need todo is ensure that the
thread is started after clone(). This avoids the async
signal safety requirement from fork()+exec(), because the
parent is single threaded at the point memory is made to
be copy-on-write. So there is no need to synchronize
with any calls in the child proccess

> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index c000a82..27bdcc0 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -1844,9 +1844,24 @@ static int lxcContainerChild(void *data)
>      cmd = lxcContainerBuildInitCmd(vmDef);
>      virCommandWriteArgLog(cmd, 1);
>  
> +    /* call setxid before libvirt controller creates fuse thread. */
>      if (lxcContainerSetID(vmDef) < 0)
>          goto cleanup;
>  
> +    /* rename and enable interfaces */
> +    if (lxcContainerRenameAndEnableInterfaces(!!(vmDef->features &
> +                                                 (1 << VIR_DOMAIN_FEATURE_PRIVNET)),
> +                                              argv->nveths,
> +                                              argv->veths) < 0) {
> +        goto cleanup;
> +    }
> +
> +    if (lxcContainerSendContinue(argv->handshakefd) < 0) {
> +        virReportSystemError(errno, "%s",
> +                            _("Failed to send continue signal to controller"));
> +        goto cleanup;
> +    }

Moving the SendContinue call means that we loose all error reporting from
this point onwards.

> +
>      root = virDomainGetRootFilesystem(vmDef);
>  
>      if (argv->nttyPaths) {
> @@ -1886,24 +1901,10 @@ static int lxcContainerChild(void *data)
>          goto cleanup;
>      }
>  
> -    /* rename and enable interfaces */
> -    if (lxcContainerRenameAndEnableInterfaces(!!(vmDef->features &
> -                                                 (1 << VIR_DOMAIN_FEATURE_PRIVNET)),
> -                                              argv->nveths,
> -                                              argv->veths) < 0) {
> -        goto cleanup;
> -    }

This patch wasn't created against current git master, since this code
has been changed

> -
>      /* drop a set of root capabilities */
>      if (lxcContainerDropCapabilities(!!hasReboot) < 0)
>          goto cleanup;
>  
> -    if (lxcContainerSendContinue(argv->handshakefd) < 0) {
> -        virReportSystemError(errno, "%s",
> -                            _("Failed to send continue signal to controller"));
> -        goto cleanup;
> -    }
> -
>      VIR_DEBUG("Setting up security labeling");
>      if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
>          goto cleanup;


In fact all these changes in this file are bogus.

> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index c8f68c0..5d1ec49 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -1981,6 +1981,12 @@ virLXCControllerSetupFuse(virLXCControllerPtr ctrl)
>  }
>  
>  static int
> +virLXCControllerStartFuse(virLXCControllerPtr ctrl)
> +{
> +    return lxcStartFuse(ctrl->fuse);
> +}
> +
> +static int
>  virLXCControllerSetupConsoles(virLXCControllerPtr ctrl,
>                                char **containerTTYPaths)
>  {
> @@ -2197,7 +2203,9 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
>          goto cleanup;
>      }
>  
> -    /* Now the container is fully setup... */
> +    /* container has already called setxid, we can create thread now.*/
> +    if (virLXCControllerStartFuse(ctrl) < 0)
> +        goto cleanup;
>  
>      /* ...and reduce our privileges */
>      if (lxcControllerClearCapabilities() < 0)
> diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c
> index 9d12832..88e122e 100644
> --- a/src/lxc/lxc_fuse.c
> +++ b/src/lxc/lxc_fuse.c
> @@ -322,12 +322,6 @@ int lxcSetupFuse(virLXCFusePtr *f, virDomainDefPtr def)
>          goto cleanup1;
>      }
>  
> -    if (virThreadCreate(&fuse->thread, false, lxcFuseRun,
> -                        (void *)fuse) < 0) {
> -        lxcFuseDestroy(fuse);
> -        goto cleanup1;
> -    }
> -
>      ret = 0;
>  cleanup:
>      fuse_opt_free_args(&args);
> @@ -341,6 +335,17 @@ cleanup2:
>      goto cleanup;
>  }
>  
> +int lxcStartFuse(virLXCFusePtr fuse)
> +{
> +    if (virThreadCreate(&fuse->thread, false, lxcFuseRun,
> +                        (void *)fuse) < 0) {
> +        lxcFuseDestroy(fuse);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  void lxcFreeFuse(virLXCFusePtr *f)
>  {
>      virLXCFusePtr fuse = *f;
> @@ -364,6 +369,10 @@ int lxcSetupFuse(virLXCFusePtr *f ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +int lxcStartFuse(virLXCFusePtr f ATTRIBUTE_UNUSED)
> +{
> +}
> +
>  void lxcFreeFuse(virLXCFusePtr *f ATTRIBUTE_UNUSED)
>  {
>  }
> diff --git a/src/lxc/lxc_fuse.h b/src/lxc/lxc_fuse.h
> index b3713af..d60492b 100644
> --- a/src/lxc/lxc_fuse.h
> +++ b/src/lxc/lxc_fuse.h
> @@ -58,6 +58,7 @@ struct virLXCFuse {
>  typedef struct virLXCFuse *virLXCFusePtr;
>  
>  extern int lxcSetupFuse(virLXCFusePtr *f, virDomainDefPtr def);
> +extern int lxcStartFuse(virLXCFusePtr f);
>  extern void lxcFreeFuse(virLXCFusePtr *f);
>  

I've copied you on a much simpler patch which avoids the race in my
testing.

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