On Wed, Oct 5, 2016 at 12:09 PM, Dmitry Torokhov
[off-list ref] wrote:

[ Some comments are form Ricky Zhou [off-list ref], some from
myself ]
On Mon, Oct 03, 2016 at 09:41:29PM -0700, John Stultz wrote:

quoted

From: Colin Cross <redacted>

[snip]

quoted

+
+             cset = task_css_set(task);

Do we need to take css_set_lock here? If not, why?

quoted

+             list_add(&cset->mg_node, &tset.src_csets);
+             ret = cgroup_allow_attach(dst_cgrp, &tset);
+             list_del(&tset.src_csets);

This should be

                list_del_init(&cset->mg_node);

since you are deleting task's cset from the tset list, not other way
around. It only happen to work because there is exactly 1 member in
tset.src_csets and list_del done on it is exactly list_del_init on the
node, so you are not leaving with uncorrupted mg_node in task's cset.

quoted

+             if (ret)
+                     ret = -EACCES;
+     }

      if (!ret && cgroup_on_dfl(dst_cgrp)) {
              struct super_block *sb = of->file->f_path.dentry->d_sb;

Isn't this, generally speaking, racy? We take current task's cset and
check if we have rights to move it over.  But we do not have any locking
between check and actual move, so can the cset change between these 2
operations?

And if cset can't really change and it is only 1 task, then why do we
bother with forming taskset at all? Can we make allow_attach take just
the target task argument?

After Tejun's feedback, I've tried reworking the same functionality in
a much simpler fashion by introducing a new capability bit.
https://lkml.org/lkml/2016/10/4/479

I believe that approach doesn't have the drawbacks you've pointed out
here, but would appreciate your input on it.

As for your feedback on this patch, I'll have to look into it a bit,
as I don't have good answers for you for you right off.  But these do
seem like valid concerns and since the Android common.git kernels are
using the code I submitted here, this issues likely need to be fixed
there.

thanks
-john