Re: [PATCH] cgroups: fix API thinko

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 06 Aug 2010 10:38:24 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Fri, 2010-08-06 at 09:34 -0700, Sridhar Samudrala wrote:
> > On 8/5/2010 3:59 PM, Michael S. Tsirkin wrote:
> > > cgroup_attach_task_current_cg API that have upstream is backwards: we
> > > really need an API to attach to the cgroups from another process A to
> > > the current one.
> > >
> > > In our case (vhost), a priveledged user wants to attach it's task to cgroups
> > > from a less priveledged one, the API makes us run it in the other
> > > task's context, and this fails.
> > >
> > > So let's make the API generic and just pass in 'from' and 'to' tasks.
> > > Add an inline wrapper for cgroup_attach_task_current_cg to avoid
> > > breaking bisect.
> > >
> > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> > > ---
> > >
> > > Paul, Li, Sridhar, could you please review the following
> > > patch?
> > >
> > > I only compile-tested it due to travel, but looks
> > > straight-forward to me.
> > > Alex Williamson volunteered to test and report the results.
> > > Sending out now for review as I might be offline for a bit.
> > > Will only try to merge when done, obviously.
> > >
> > > If OK, I would like to merge this through -net tree,
> > > together with the patch fixing vhost-net.
> > > Let me know if that sounds ok.
> > >
> > > Thanks!
> > >
> > > This patch is on top of net-next, it is needed for fix
> > > vhost-net regression in net-next, where a non-priveledged
> > > process can't enable the device anymore:
> > >
> > > when qemu uses vhost, inside the ioctl call it
> > > creates a thread, and tries to add
> > > this thread to the groups of current, and it fails.
> > > But we control the thread, so to solve the problem,
> > > we really should tell it 'connect to out cgroups'.

So am I correct to assume that this change is now needed in 2.6.36, and
unneeded in 2.6.35?

Can it affect the userspace<->kernel API in amy manner?  If so, it
should be backported into earlier kernels to reduce the number of
incompatible kernels out there.

Paul, did you have any comments?

I didn't see any update in response to the minor review comments, so...


 include/linux/cgroup.h |    1 +
 kernel/cgroup.c        |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff -puN include/linux/cgroup.h~cgroups-fix-api-thinko-fix include/linux/cgroup.h
--- a/include/linux/cgroup.h~cgroups-fix-api-thinko-fix
+++ a/include/linux/cgroup.h
@@ -579,6 +579,7 @@ void cgroup_iter_end(struct cgroup *cgrp
 int cgroup_scan_tasks(struct cgroup_scanner *scan);
 int cgroup_attach_task(struct cgroup *, struct task_struct *);
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
+
 static inline int cgroup_attach_task_current_cg(struct task_struct *tsk)
 {
 	return cgroup_attach_task_all(current, tsk);
diff -puN kernel/cgroup.c~cgroups-fix-api-thinko-fix kernel/cgroup.c
--- a/kernel/cgroup.c~cgroups-fix-api-thinko-fix
+++ a/kernel/cgroup.c
@@ -1798,13 +1798,13 @@ out:
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk)
 {
 	struct cgroupfs_root *root;
-	struct cgroup *cur_cg;
 	int retval = 0;
 
 	cgroup_lock();
 	for_each_active_root(root) {
-		cur_cg = task_cgroup_from_root(from, root);
-		retval = cgroup_attach_task(cur_cg, tsk);
+		struct cgroup *from_cg = task_cgroup_from_root(from, root);
+
+		retval = cgroup_attach_task(from_cg, tsk);
 		if (retval)
 			break;
 	}
_