MIME-Version: 1.0
In-Reply-To: <20161005190909.GA31873@dtor-ws>
References: <1475556090-6278-1-git-send-email-john.stultz@linaro.org>
 <1475556090-6278-2-git-send-email-john.stultz@linaro.org> <20161005190909.GA31873@dtor-ws>
From: John Stultz <john.stultz@linaro.org>
Date: Wed, 5 Oct 2016 12:16:48 -0700
Message-ID: <CALAqxLWUGmMWBu54uRgK2tn9EuCRSDMPHewDmB5DEP+c-WaJEQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] cgroup: Add generic cgroup subsystem permission checks
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: lkml <linux-kernel@vger.kernel.org>, Colin Cross <ccross@android.com>,
        Tejun Heo <tj@kernel.org>, Li Zefan <lizefan@huawei.com>,
        Jonathan Corbet <corbet@lwn.net>, cgroups@vger.kernel.org,
        Android Kernel Team <kernel-team@android.com>,
        Rom Lemarchand <romlem@android.com>,
        Dmitry Shmidt <dimitrysh@google.com>, Todd Kjos <tkjos@google.com>,
        Christian Poetzsch <christian.potzsch@imgtec.com>,
        Amit Pundir <amit.pundir@linaro.org>, Ricky Zhou <rickyz@chromium.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2107
Lines: 57

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