Re: [PATCH] lxc: Add virCgroupSetOwner()

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

 



On Fri, Feb 14, 2014 at 08:47:37AM +0100, Richard Weinberger wrote:
> 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@xxxxxx>
> >>
> >> 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@xxxxxx>
> >> ---
> >>  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?
>

I'm not saying either way, but if you're not using the error (or you
don't want that error to be used, than don't report it with
virReportError() and use VIR_WARN() for example.  However, if the
called function should report an error and this is the only case
which should not do it (an exception), then reset the error at least.

> >>  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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]