Re: [libvirt] [PATCH] lxc: Add virCgroupSetOwner()

Am 14.02.2014 08:10, schrieb Martin Kletzander: > On Thu, Feb 13, 2014 at 05:15:22PM +0000, Daniel P. Berrange wrote: >> From: Richard Weinberger <richard(a)nod.at&gt; >> >> Add a new helper function to change the permissions of a control >> group. This function is needed for user namespaces, we need to >> chmod() the cgroup to the initial uid/gid such that systemd is >> allowed to use the cgroup. >> >> Only the systemd controller is made accessible to the container. >> Others must remain read-only since it is generally not safe >> to delegate resource controller write access to unprivileged >> processes. >> >> Signed-off-by: Richard Weinberger <richard(a)nod.at&gt; >> --- >> src/libvirt_private.syms | 1 + >> src/lxc/lxc_cgroup.c | 9 ++++++++ >> src/util/vircgroup.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ >> src/util/vircgroup.h | 5 +++++ >> 4 files changed, 69 insertions(+) >> >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> index 0b28bac..cfa9f75 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -1056,6 +1056,7 @@ virCgroupSetMemory; >> virCgroupSetMemoryHardLimit; >> virCgroupSetMemorySoftLimit; >> virCgroupSetMemSwapHardLimit; >> +virCgroupSetOwner; >> virCgroupSupportsCpuBW; >> >> >> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c >> index cc0d5e8..0d0d9c0 100644 >> --- a/src/lxc/lxc_cgroup.c >> +++ b/src/lxc/lxc_cgroup.c >> @@ -484,6 +484,15 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def) >> &cgroup) < 0) >> goto cleanup; >> >> + /* setup control group permissions for user namespace */ >> + if (def->idmap.uidmap) { >> + if (virCgroupSetOwner(cgroup, >> + def->idmap.uidmap[0].target, >> + def->idmap.gidmap[0].target, >> + (1 << VIR_CGROUP_CONTROLLER_SYSTEMD))) > > This should be "if (virCgroupSetOwner() < 0)" to go with the rest. Ok. >> + goto cleanup; >> + } >> + > > virCgroupNewMachine() guarantees that the cgroup is NULL in case of an > error, but you don't guarantee that in virCgroupSetOwner(), so the > errors from it won't propagate anywhere, because you don't return NULL > from this function. Do we really want to treat a failed chown() as fatal error?

>> cleanup: >> return cgroup; >> } >> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c >> index a6d60c5..2dc6986 100644 >> --- a/src/util/vircgroup.c >> +++ b/src/util/vircgroup.c >> @@ -3253,6 +3253,60 @@ cleanup: >> } >> >> >> +int virCgroupSetOwner(virCgroupPtr cgroup, >> + uid_t uid, >> + gid_t gid, >> + int controllers) >> +{ >> + size_t i; >> + >> + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { >> + char *base, *entry; >> + DIR *dh; >> + struct dirent *de; >> + >> + if (!((1 << i) & controllers)) >> + continue; >> + >> + if (!cgroup->controllers[i].mountPoint) >> + continue; >> + >> + if (virAsprintf(&base, "%s%s", cgroup->controllers[i].mountPoint, >> + cgroup->controllers[i].placement) < 0) { >> + virReportOOMError(); > > Double OOM reporting. Ahh, virAsprintf() already reports the error... >> + return -1; >> + } >> + >> + dh = opendir(base); >> + while ((de = readdir(dh)) != NULL) { >> + if (STREQ(de->d_name, ".") || >> + STREQ(de->d_name, "..")) >> + continue; >> + >> + if (virAsprintf(&entry, "%s/%s", base, de->d_name) < 0) { >> + VIR_FREE(base); >> + virReportOOMError(); > > Same here, plus you continue the loop and don't return -1. Ok! >> + } >> + >> + if (chown(entry, uid, gid) < 0) >> + virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), >> + entry, uid, gid); > > Indentation's off and you continue the loop again. I continue here by design because I don't treat a failed chown() as fatal error. >> + >> + VIR_FREE(entry); >> + } >> + closedir(dh); >> + >> + if (chown(base, uid, gid) < 0) >> + virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), >> + base, uid, gid); > > Again reporting an error, but returning 0 even in case of an error. Same here. Thanks, //richard -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list