11:37 a.m.
These two patches fix the issue that control groups are unusable if
user namespaces are enabled.
We have to chown() the control group to the correct user.
As the container mounts the control group and only the controller
is allowed to chown() the mount point we need a new barrier to synchronize
them after the container has setup the control groups.
Thanks,
//richard
[PATCH 1/2] lxc: Add another barrier
[PATCH 2/2] lxc: Add virCgroupSetOwner()
11:37 a.m.
New subject: [libvirt] [PATCH 1/2] lxc: Add another barrier
Add another barrier to give the controller a chance to
setup additional things after the container setup is done.
This new barrier is needed to chown() the cgroup after
the container has mounted it.
Signed-off-by: Richard Weinberger <richard(a)nod.at>
---
src/lxc/lxc_container.c | 16 +++++++++++++++-
src/lxc/lxc_container.h | 1 +
src/lxc/lxc_controller.c | 17 +++++++++++++++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index c6bdc8c..24af73a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -108,6 +108,7 @@ struct __lxc_child_argv {
size_t nttyPaths;
char **ttyPaths;
int handshakefd;
+ int posthandshakefd;
};
static int lxcContainerMountFSBlock(virDomainFSDefPtr fs,
@@ -1880,10 +1881,20 @@ static int lxcContainerChild(void *data)
goto cleanup;
}
+ /* wait for controller to setup final tasks */
+ VIR_DEBUG("Received container continue message");
+ if (lxcContainerWaitForContinue(argv->posthandshakefd) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Failed to read the container continue
message"));
+ goto cleanup;
+ }
+ VIR_DEBUG("Received container continue message");
+
VIR_DEBUG("Setting up security labeling");
if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0)
goto cleanup;
+ VIR_FORCE_CLOSE(argv->posthandshakefd);
VIR_FORCE_CLOSE(argv->handshakefd);
VIR_FORCE_CLOSE(argv->monitor);
if (lxcContainerSetupFDs(&ttyfd,
@@ -1896,6 +1907,7 @@ cleanup:
VIR_FORCE_CLOSE(ttyfd);
VIR_FORCE_CLOSE(argv->monitor);
VIR_FORCE_CLOSE(argv->handshakefd);
+ VIR_FORCE_CLOSE(argv->posthandshakefd);
if (ret == 0) {
/* this function will only return if an error occurred */
@@ -1984,6 +1996,7 @@ int lxcContainerStart(virDomainDefPtr def,
int *passFDs,
int control,
int handshakefd,
+ int posthandshakefd,
size_t nttyPaths,
char **ttyPaths)
{
@@ -2001,7 +2014,8 @@ int lxcContainerStart(virDomainDefPtr def,
.monitor = control,
.nttyPaths = nttyPaths,
.ttyPaths = ttyPaths,
- .handshakefd = handshakefd
+ .handshakefd = handshakefd,
+ .posthandshakefd = posthandshakefd
};
/* allocate a stack for the container */
diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
index e74a7d7..03102f4 100644
--- a/src/lxc/lxc_container.h
+++ b/src/lxc/lxc_container.h
@@ -60,6 +60,7 @@ int lxcContainerStart(virDomainDefPtr def,
int *passFDs,
int control,
int handshakefd,
+ int posthandshakefd,
size_t nttyPaths,
char **ttyPaths);
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 5ca960f..f7b614b 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2128,6 +2128,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
int rc = -1;
int control[2] = { -1, -1};
int containerhandshake[2] = { -1, -1 };
+ int containerposthandshake[2] = { -1, -1 };
char **containerTTYPaths = NULL;
size_t i;
@@ -2146,6 +2147,12 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
goto cleanup;
}
+ if (socketpair(PF_UNIX, SOCK_STREAM, 0, containerposthandshake) < 0) {
+ virReportSystemError(errno, "%s",
+ _("socketpair failed"));
+ goto cleanup;
+ }
+
if (virLXCControllerSetupPrivateNS() < 0)
goto cleanup;
@@ -2184,11 +2191,13 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
ctrl->passFDs,
control[1],
containerhandshake[1],
+ containerposthandshake[1],
ctrl->nconsoles,
containerTTYPaths)) < 0)
goto cleanup;
VIR_FORCE_CLOSE(control[1]);
VIR_FORCE_CLOSE(containerhandshake[1]);
+ VIR_FORCE_CLOSE(containerposthandshake[1]);
for (i = 0; i < ctrl->npassFDs; i++)
VIR_FORCE_CLOSE(ctrl->passFDs[i]);
@@ -2214,6 +2223,12 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
goto cleanup;
}
+ if (lxcContainerSendContinue(containerposthandshake[0]) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to send container continue message"));
+ goto cleanup;
+ }
+
/* ...and reduce our privileges */
if (lxcControllerClearCapabilities() < 0)
goto cleanup;
@@ -2240,6 +2255,8 @@ cleanup:
VIR_FORCE_CLOSE(control[1]);
VIR_FORCE_CLOSE(containerhandshake[0]);
VIR_FORCE_CLOSE(containerhandshake[1]);
+ VIR_FORCE_CLOSE(containerposthandshake[0]);
+ VIR_FORCE_CLOSE(containerposthandshake[1]);
for (i = 0; i < ctrl->nconsoles; i++)
VIR_FREE(containerTTYPaths[i]);
--
1.8.4.5
11:37 a.m.
New subject: [libvirt] [PATCH 2/2] lxc: Add virCgroupSetOwner()
Add a new helper function to change the permissions
of a control group.
Signed-off-by: Richard Weinberger <richard(a)nod.at>
---
src/lxc/lxc_controller.c | 7 +++++++
src/util/vircgroup.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
3 files changed, 52 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index f7b614b..6e348b3 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2223,6 +2223,13 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
goto cleanup;
}
+ /* setup control group permissions for user namespace */
+ if (ctrl->def->idmap.uidmap) {
+ if (virCgroupSetOwner(ctrl->cgroup, ctrl->def->idmap.uidmap[0].target,
+ ctrl->def->idmap.gidmap[0].target))
+ goto cleanup;
+ }
+
if (lxcContainerSendContinue(containerposthandshake[0]) < 0) {
virReportSystemError(errno, "%s",
_("Unable to send container continue message"));
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a6d60c5..b66ffed 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3252,6 +3252,49 @@ cleanup:
return ret;
}
+int virCgroupSetOwner(virCgroupPtr cgroup, uid_t uid, gid_t gid) {
+ size_t i;
+
+ for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
+ char *base, *entry;
+ DIR *dh;
+ struct dirent *de;
+
+ if (virAsprintf(&base, "%s%s",
cgroup->controllers[i].mountPoint,
+ cgroup->controllers[i].placement) < 0) {
+ virReportOOMError();
+ 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();
+ }
+
+ if (chown(entry, uid, gid) < 0)
+ virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
+ entry, uid, gid);
+
+ VIR_FREE(entry);
+ }
+ closedir(dh);
+
+ if (chown(base, uid, gid) < 0)
+ virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
+ base, uid, gid);
+
+ VIR_FREE(base);
+ }
+
+ return 0;
+}
+
/**
* virCgroupSupportsCpuBW():
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index a70eb18..6e00f28 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -225,4 +225,6 @@ int virCgroupIsolateMount(virCgroupPtr group,
bool virCgroupSupportsCpuBW(virCgroupPtr cgroup);
+int virCgroupSetOwner(virCgroupPtr cgroup, uid_t uid, gid_t gid);
+
#endif /* __VIR_CGROUP_H__ */
--
1.8.4.5
6:05 a.m.
New subject: [libvirt] [PATCH 2/2] lxc: Add virCgroupSetOwner()
On Sat, Feb 08, 2014 at 06:37:43PM +0100, Richard Weinberger wrote:
Add a new helper function to change the permissions
of a control group.
Signed-off-by: Richard Weinberger <richard(a)nod.at>
---
src/lxc/lxc_controller.c | 7 +++++++
src/util/vircgroup.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/util/vircgroup.h | 2 ++
3 files changed, 52 insertions(+)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index f7b614b..6e348b3 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2223,6 +2223,13 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
goto cleanup;
}
+ /* setup control group permissions for user namespace */
+ if (ctrl->def->idmap.uidmap) {
+ if (virCgroupSetOwner(ctrl->cgroup, ctrl->def->idmap.uidmap[0].target,
+ ctrl->def->idmap.gidmap[0].target))
+ goto cleanup;
+ }
+
I wonder is it possible to just do the ownership change in the
virLXCCgroupCreate method ? When the container then does the
bind mount, it should preserve the ownership correctly. This
would avoid any need for the extra handshake synchronization.
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index a6d60c5..b66ffed 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3252,6 +3252,49 @@ cleanup:
return ret;
}
+int virCgroupSetOwner(virCgroupPtr cgroup, uid_t uid, gid_t gid) {
+ size_t i;
+
+ for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
I think we only want to set ownership for VIR_CGROUP_CONTROLLER_SYSTEMD.
We don't want to give the container permission to change resource
controllers, this that would allow it to potentially escape its resource
limits. You might think the hierarchy would lock things down, but I
believe the kernel developers still say it is unsafe to delegate cgroup
resource controllers to unprivileged users.
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 :|
4:51 p.m.
New subject: [libvirt] [PATCH 2/2] lxc: Add virCgroupSetOwner()
Am 11.02.2014 13:05, schrieb Daniel P. Berrange:
On Sat, Feb 08, 2014 at 06:37:43PM +0100, Richard Weinberger wrote:
> Add a new helper function to change the permissions
> of a control group.
>
> Signed-off-by: Richard Weinberger <richard(a)nod.at>
> ---
> src/lxc/lxc_controller.c | 7 +++++++
> src/util/vircgroup.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> src/util/vircgroup.h | 2 ++
> 3 files changed, 52 insertions(+)
>
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index f7b614b..6e348b3 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -2223,6 +2223,13 @@ virLXCControllerRun(virLXCControllerPtr ctrl)
> goto cleanup;
> }
>
> + /* setup control group permissions for user namespace */
> + if (ctrl->def->idmap.uidmap) {
> + if (virCgroupSetOwner(ctrl->cgroup,
ctrl->def->idmap.uidmap[0].target,
> + ctrl->def->idmap.gidmap[0].target))
> + goto cleanup;
> + }
> +
I wonder is it possible to just do the ownership change in the
virLXCCgroupCreate method ? When the container then does the
bind mount, it should preserve the ownership correctly. This
would avoid any need for the extra handshake synchronization.
Bah, I've overlooked that. Of course it works.
Much simplified patch is on its way.
Thanks for pointing this out!
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index a6d60c5..b66ffed 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -3252,6 +3252,49 @@ cleanup:
> return ret;
> }
>
> +int virCgroupSetOwner(virCgroupPtr cgroup, uid_t uid, gid_t gid) {
> + size_t i;
> +
> + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
I think we only want to set ownership for VIR_CGROUP_CONTROLLER_SYSTEMD.
We don't want to give the container permission to change resource
controllers, this that would allow it to potentially escape its resource
limits. You might think the hierarchy would lock things down, but I
believe the kernel developers still say it is unsafe to delegate cgroup
resource controllers to unprivileged users.
That's right. But then we should generally only delegate the systemd cgroup
to a container.
...patch sent.
Thanks,
//richard
3937
days inactive
3940
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Richard Weinberger