[libvirt] [PATCH v2] lxc: Add virCgroupSetOwner()
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. Signed-off-by: Richard Weinberger <richard@nod.at> --- Changes between v1 and v2: - Addressed Martin Kletzander's comments - Fixed opendir() error handling --- src/libvirt_private.syms | 1 + src/lxc/lxc_cgroup.c | 12 ++++++++++ src/util/vircgroup.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 5 ++++ 4 files changed, 78 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2c9536a..40e72f2 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..39d955c 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -484,6 +484,18 @@ 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)) < 0) { + virCgroupFree(&cgroup); + cgroup = NULL; + goto cleanup; + } + } + cleanup: return cgroup; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a6d60c5..4bef0db 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3253,6 +3253,66 @@ 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) { + return -1; + } + + dh = opendir(base); + if (!dh) { + VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno)); + VIR_FREE(base); + return -1; + } + + 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); + closedir(dh); + return -1; + } + + if (chown(entry, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno)); + + VIR_FREE(entry); + } + closedir(dh); + + if (chown(base, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno)); + + VIR_FREE(base); + } + + return 0; +} + + /** * virCgroupSupportsCpuBW(): * Check whether the host supports CFS bandwidth. diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index a70eb18..38d94f3 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -225,4 +225,9 @@ int virCgroupIsolateMount(virCgroupPtr group, bool virCgroupSupportsCpuBW(virCgroupPtr cgroup); +int virCgroupSetOwner(virCgroupPtr cgroup, + uid_t uid, + gid_t gid, + int controllers); + #endif /* __VIR_CGROUP_H__ */ -- 1.8.4.5
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a6d60c5..4bef0db 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3253,6 +3253,66 @@ 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) { + return -1; + }
Indentation of 'return' is too deep
+ + dh = opendir(base); + if (!dh) { + VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno)); + VIR_FREE(base); + return -1; + }
This should use virReportSystemError.
+ + 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); + closedir(dh); + return -1; + } + + if (chown(entry, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno));
This should use virReportSystemError too, and propagate the error.
+ + VIR_FREE(entry); + } + closedir(dh); + + if (chown(base, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno));
Likewise. Also it uses NULL 'entry' var rather than 'base' in the message args.
+ + VIR_FREE(base); + } + + return 0; +}
I'm going to send a followup with these fixes for review, which also centralizes the error cleanup code. Regards, 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 :|
Am 24.02.2014 13:20, schrieb Daniel P. Berrange:
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a6d60c5..4bef0db 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3253,6 +3253,66 @@ 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) { + return -1; + }
Indentation of 'return' is too deep
Do you have something like a checkpatch.pl? ;=)
+ + dh = opendir(base); + if (!dh) { + VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno)); + VIR_FREE(base); + return -1; + }
This should use virReportSystemError.
To avoid further confusion. When to use VIR_ERROR() and when virReportSystemError()?
+ + 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); + closedir(dh); + return -1; + } + + if (chown(entry, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno));
This should use virReportSystemError too, and propagate the error.
Do you we really want to propagate this error? IMHO a failing chown() is not a fatal error.
+ + VIR_FREE(entry); + } + closedir(dh); + + if (chown(base, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno));
Likewise. Also it uses NULL 'entry' var rather than 'base' in the message args.
+ + VIR_FREE(base); + } + + return 0; +}
I'm going to send a followup with these fixes for review, which also centralizes the error cleanup code.
Okay! Thanks, //richard
On Mon, Feb 24, 2014 at 01:25:04PM +0100, Richard Weinberger wrote:
Am 24.02.2014 13:20, schrieb Daniel P. Berrange:
On Fri, Feb 14, 2014 at 02:25:55PM +0100, Richard Weinberger wrote:
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a6d60c5..4bef0db 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3253,6 +3253,66 @@ 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) { + return -1; + }
Indentation of 'return' is too deep
Do you have something like a checkpatch.pl? ;=)
Just my eyes in this case ;-P
+ dh = opendir(base); + if (!dh) { + VIR_ERROR(_("Unable to open %s: %s"), base, strerror(errno)); + VIR_FREE(base); + return -1; + }
This should use virReportSystemError.
To avoid further confusion. When to use VIR_ERROR() and when virReportSystemError()?
VIR_ERROR merely puts a message in the logs. It doesn't propagate anything back to the client making the API call. The virReport*Error functions actually send an error back to the client app. You basically always want virReport*Error - there's almost no cases where VIR_ERROR is the right thing todo.
+ + 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); + closedir(dh); + return -1; + } + + if (chown(entry, uid, gid) < 0) + VIR_WARN(_("cannot chown '%s' to (%u, %u): %s"), entry, uid, gid, + strerror(errno));
This should use virReportSystemError too, and propagate the error.
Do you we really want to propagate this error? IMHO a failing chown() is not a fatal error.
I don't see a valid reason why chown would fail in normal usage, so I think it should be fatal. Regards, 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 :|
participants (2)
-
Daniel P. Berrange -
Richard Weinberger