[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]