On Tue, Oct 15, 2013 at 06:41:28PM +0800, Chen Hanxiao wrote: > ping... > > > -----Original Message----- > > From: libvir-list-bounces@xxxxxxxxxx > [mailto:libvir-list-bounces@xxxxxxxxxx] > > On Behalf Of Chen Hanxiao > > Sent: Wednesday, October 09, 2013 6:03 PM > > To: libvir-list@xxxxxxxxxx > > Subject: Re: [PATCH v3]LXC: Helper function for checking > permission of > > dir when userns enabled > > > > ping? > > > > > -----Original Message----- > > > From: Chen Hanxiao [mailto:chenhanxiao@xxxxxxxxxxxxxx] > > > Sent: Tuesday, September 10, 2013 4:18 PM > > > To: libvir-list@xxxxxxxxxx > > > Cc: chenhanxiao@xxxxxxxxxxxxxx > > > Subject: [PATCH v3]LXC: Helper function for checking permission > of dir > > > when userns enabled > > > > > > From: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > > > > > If we enable userns, the process with uid/gid in idmap > > > should have enough permission to access dir we provided > > > for containers. > > > Currently, the debug log is very implicit > > > or misleading sometimes. > > > This patch will help clarify this for us > > > when using debug log or virsh. > > > > > > v2: syntax-check clean > > > > > > v3: reliable method for checking permission of dir > > > > > > Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > > --- > > > src/lxc/lxc_container.c | 88 > > > +++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 88 insertions(+) > > > > > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > > > index 8abaea0..9a05e30 100644 > > > --- a/src/lxc/lxc_container.c > > > +++ b/src/lxc/lxc_container.c > > > @@ -110,6 +110,13 @@ struct __lxc_child_argv { > > > int handshakefd; > > > }; > > > > > > +typedef struct __lxc_userns_DirPermCheck_argv > > > lxc_userns_DirPermCheck_argv_t; > > > +struct __lxc_userns_DirPermCheck_argv { > > > + uid_t uid; > > > + gid_t gid; > > > + virDomainDefPtr vmDef; > > > +}; > > > + > > > static int lxcContainerMountFSBlock(virDomainFSDefPtr fs, > > > const char *srcprefix); > > > > > > @@ -1829,6 +1836,84 @@ lxcNeedNetworkNamespace(virDomainDefPtr > > def) > > > return false; > > > } > > > > > > +static > > > +int lxcContainerCheckDirPermissionChild(void *argv) > > > +{ > > > + size_t i; > > > + lxc_userns_DirPermCheck_argv_t *args = argv; > > > + uid_t uid = args->uid; > > > + uid_t gid = args->gid; > > > + virDomainDefPtr vmDef = args->vmDef; > > > + char *path; > > > + > > > + if (virSetUIDGID(uid, gid, NULL, 0) < 0) { > > > + virReportSystemError(errno, "%s", > > > + _("setuid or setgid failed")); > > > + _exit(-1); > > > + } > > > + > > > + for (i = 0; i < vmDef->nfss; i++) { > > > + path = vmDef->fss[i]->src; > > > + if (access(path, R_OK) || access(path, W_OK) || > > > virFileIsExecutable(path)) { > > > + VIR_DEBUG("Src dir '%s' does not belong to uid/gid: %d/%d", > > > + vmDef->fss[i]->src, uid, gid); > > > + _exit(-1); > > > + } > > > + } > > > + > > > + _exit(0); > > > +} > > > + > > > +/* > > > + * Helper function for helping check > > > + * whether we have enough privilege > > > + * to operate the source dir when userns enabled > > > + * @vmDef: pointer to vm definition structure > > > + * Returns 0 on success or -1 in case of error > > > + */ > > > +static int > > > +lxcContainerCheckDirPermission(virDomainDefPtr vmDef) > > > +{ > > > + uid_t uid; > > > + gid_t gid; > > > + int cpid = 0; > > > + int status; > > > + char *childStack; > > > + char *stack; > > > + int flags = SIGCHLD; > > > + > > > + uid = vmDef->idmap.uidmap[0].target; > > > + gid = vmDef->idmap.gidmap[0].target; > > > + > > > + lxc_userns_DirPermCheck_argv_t args = { > > > + .uid = uid, > > > + .gid = gid, > > > + .vmDef = vmDef > > > + }; > > > + > > > + if (VIR_ALLOC_N(stack, getpagesize() * 4) < 0) > > > + return -1; > > > + > > > + childStack = stack + (getpagesize() * 4); > > > + cpid = clone(lxcContainerCheckDirPermissionChild, childStack, > flags, > > > &args); > > > + VIR_FREE(stack); > > > + if (cpid < 0) { > > > + virReportSystemError(errno, "%s", > > > + _("Unable to clone to check permission > > of > > > directory")); > > > + return -1; > > > + } else if (virProcessWait(cpid, &status) < 0) { > > > + return -1; > > > + } > > > + > > > + if (WEXITSTATUS(status) != 0) { > > > + virReportSystemError(errno, "%s", > > > + _("Check the permission of source dir > > > provided for container")); > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * lxcContainerStart: > > > * @def: pointer to virtual machine structure > > > @@ -1880,6 +1965,9 @@ int lxcContainerStart(virDomainDefPtr def, > > > if (userns_supported()) { > > > VIR_DEBUG("Enable user namespace"); > > > cflags |= CLONE_NEWUSER; > > > + if (lxcContainerCheckDirPermission(def) < 0) { > > > + return -1; > > > + } > > > } else { > > > virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED, > > > "%s", > > > _("Kernel doesn't support user > > > namespace")); > > > -- > > > 1.8.2.1 This patch is basically about improving the error message from LXC when permissions are inadequate. Over the past week I've pushed a large number of patches to fix LXC error reporting during startup. As such, I'm not sure that we still need to have this patch - can you try latest GIT master and show what error message you get with existing code. I'd really rather avoid checking permissions ourselves, if we have got error reporting at time of use working properly 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list