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