Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1496005

Re: [PATCH 1/2] cgroup: Add generic cgroup subsystem permission checks

Show all headers | View raw

On Wed, Oct 5, 2016 at 12:09 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> [ Some comments are form Ricky Zhou <rickyz@chromium.org>, some from
> myself ]
> On Mon, Oct 03, 2016 at 09:41:29PM -0700, John Stultz wrote:
>> From: Colin Cross <ccross@android.com>
>>
[snip]
>> +
>> +             cset = task_css_set(task);
>
> Do we need to take css_set_lock here? If not, why?
>
>> +             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.
>
>> +             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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC][PATCH 0/2] Another pass at Android style loosening of cgroup attach permissions John Stultz <john.stultz@linaro.org> - 2016-10-04 06:50 +0200
  [PATCH 2/2] cgroup: Add a allow_attach policy for Android John Stultz <john.stultz@linaro.org> - 2016-10-04 06:50 +0200
    Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android John Stultz <john.stultz@linaro.org> - 2016-10-05 21:20 +0200
      Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-07 00:50 +0200
        Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-07 01:00 +0200
    Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-05 21:20 +0200
  [PATCH 1/2] cgroup: Add generic cgroup subsystem permission checks John Stultz <john.stultz@linaro.org> - 2016-10-04 06:50 +0200
    Re: [PATCH 1/2] cgroup: Add generic cgroup subsystem permission  checks Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-05 21:10 +0200
      Re: [PATCH 1/2] cgroup: Add generic cgroup subsystem permission checks John Stultz <john.stultz@linaro.org> - 2016-10-05 21:20 +0200
        Re: [PATCH 1/2] cgroup: Add generic cgroup subsystem permission checks Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-05 21:30 +0200
  Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions Tejun Heo <tj@kernel.org> - 2016-10-04 18:20 +0200
    Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions John Stultz <john.stultz@linaro.org> - 2016-10-04 20:10 +0200
    Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions John Stultz <john.stultz@linaro.org> - 2016-10-04 20:10 +0200
      Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions Tejun Heo <tj@kernel.org> - 2016-10-04 21:40 +0200
        Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions Tejun Heo <tj@kernel.org> - 2016-10-04 21:50 +0200
        Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions John Stultz <john.stultz@linaro.org> - 2016-10-04 21:50 +0200
        Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions "Serge E. Hallyn" <serge@hallyn.com> - 2016-10-04 22:20 +0200
          Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions Tejun Heo <tj@kernel.org> - 2016-10-04 22:40 +0200
            Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions "Serge E. Hallyn" <serge@hallyn.com> - 2016-10-04 23:30 +0200
              Re: [RFC][PATCH 0/2] Another pass at Android style loosening of  cgroup attach permissions Tejun Heo <tj@kernel.org> - 2016-10-04 23:40 +0200

csiph-web