[PATCH] cgroup: missing rcu read lock around task_css_set

[PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

rcu read lock should be held when calling and working with task_css_set.

This patch also fixes a related lockdep warning.

Signed-off-by: Sasha Levin <sasha.levin-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 8f4ddbe..f638ad6 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4361,6 +4361,7 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	}
 
 	/* Reassign the task to the init_css_set. */
+	rcu_read_lock();
 	cset = task_css_set(tsk);
 	RCU_INIT_POINTER(tsk->cgroups, &init_css_set);
 
@@ -4378,6 +4379,7 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 
 	if (put_cset)
 		put_css_set(cset, true);
+	rcu_read_unlock();
 }
 
 static void check_for_release(struct cgroup *cgrp)
-- 
1.7.2.5

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Tejun Heo]

On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
> rcu read lock should be held when calling and working with task_css_set.
> 
> This patch also fixes a related lockdep warning.

Hmmm... PF_EXITING should be visible at that point and cset can't
change anymore.  We prolly need to update lockdep annotation rather
than adding spurious rcu locking around it.  Against which branch is
it?  Can you please post the lockdep warning?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

On 03/03/2014 05:33 PM, Tejun Heo wrote:
> On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
>> rcu read lock should be held when calling and working with task_css_set.
>>
>> This patch also fixes a related lockdep warning.
>
> Hmmm... PF_EXITING should be visible at that point and cset can't
> change anymore.  We prolly need to update lockdep annotation rather
> than adding spurious rcu locking around it.  Against which branch is
> it?  Can you please post the lockdep warning?

I see it on -next.

[    0.370543] ===============================
[    0.371030] [ INFO: suspicious RCU usage. ]
[    0.371453] 3.14.0-rc4-next-20140303-sasha-00012-g35a2897-dirty #43 Not tainted
[    0.372223] -------------------------------
[    0.372627] include/linux/cgroup.h:692 suspicious rcu_dereference_check() usage!
[    0.373417]
[    0.373417] other info that might help us debug this:
[    0.373417]
[    0.374223]
[    0.374223] rcu_scheduler_active = 1, debug_locks = 1
[    0.374993] no locks held by swapper/0/0.
[    0.375422]
[    0.375422] stack backtrace:
[    0.375865] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.14.0-rc4-next-20140303-sasha-00012-g35a2897-dirty #43
[    0.376936]  0000000000000001 ffffffff85e01d58 ffffffff8446a933 0000000000000001
[    0.377753]  ffffffff85e2e4a0 ffffffff85e01d88 ffffffff811a6ddb 0000000000000000
[    0.380550]  ffff88052a2398a8 ffff88052a238000 0000000000000000 ffffffff85e01de8
[    0.381370] Call Trace:
[    0.381641]  [<ffffffff8446a933>] dump_stack+0x52/0x7f
[    0.382261]  [<ffffffff811a6ddb>] lockdep_rcu_suspicious+0x10b/0x120
[    0.382893]  [<ffffffff8120024d>] cgroup_exit+0x20d/0x250
[    0.383566]  [<ffffffff811dc075>] ? ktime_get_ts+0x145/0x1d0
[    0.384221]  [<ffffffff8113fd66>] copy_process+0x5d6/0x670
[    0.384850]  [<ffffffff8114049b>] do_fork+0x8b/0x2e0
[    0.385347]  [<ffffffff811a9c0d>] ? trace_hardirqs_on+0xd/0x10
[    0.386009]  [<ffffffff8446d47e>] ? mutex_unlock+0xe/0x10
[    0.386559]  [<ffffffff872a4117>] ? early_idt_handlers+0x117/0x120
[    0.387327]  [<ffffffff81140776>] kernel_thread+0x26/0x30
[    0.387947]  [<ffffffff844606a6>] rest_init+0x26/0x150
[    0.388491]  [<ffffffff872a538d>] start_kernel+0x3c0/0x3c7
[    0.389126]  [<ffffffff872a4dfb>] ? repair_env_string+0x5b/0x5b
[    0.389813]  [<ffffffff84468dcb>] ? memblock_reserve+0x49/0x4e
[    0.390019]  [<ffffffff872a43d9>] x86_64_start_reservations+0x2a/0x2c
[    0.390754]  [<ffffffff872a45ae>] x86_64_start_kernel+0x186/0x195


Thanks,
Sasha

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Tejun Heo]

On Mon, Mar 03, 2014 at 05:43:11PM -0500, Sasha Levin wrote:
> On 03/03/2014 05:33 PM, Tejun Heo wrote:
> >On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
> >>rcu read lock should be held when calling and working with task_css_set.
> >>
> >>This patch also fixes a related lockdep warning.
> >
> >Hmmm... PF_EXITING should be visible at that point and cset can't
> >change anymore.  We prolly need to update lockdep annotation rather
> >than adding spurious rcu locking around it.  Against which branch is
> >it?  Can you please post the lockdep warning?
> 
> I see it on -next.

I think the right thing to do is using task_css_set_check() with
PF_EXITING check and add comment explaining that cset can no longer
change.  Can you please write up a patch?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

On 03/03/2014 05:45 PM, Tejun Heo wrote:
> On Mon, Mar 03, 2014 at 05:43:11PM -0500, Sasha Levin wrote:
>> On 03/03/2014 05:33 PM, Tejun Heo wrote:
>>> On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
>>>> rcu read lock should be held when calling and working with task_css_set.
>>>>
>>>> This patch also fixes a related lockdep warning.
>>>
>>> Hmmm... PF_EXITING should be visible at that point and cset can't
>>> change anymore.  We prolly need to update lockdep annotation rather
>>> than adding spurious rcu locking around it.  Against which branch is
>>> it?  Can you please post the lockdep warning?
>>
>> I see it on -next.
>
> I think the right thing to do is using task_css_set_check() with
> PF_EXITING check and add comment explaining that cset can no longer
> change.  Can you please write up a patch?

Sure!


Thanks,
Sasha

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

On 03/03/2014 05:45 PM, Tejun Heo wrote:
> On Mon, Mar 03, 2014 at 05:43:11PM -0500, Sasha Levin wrote:
>> On 03/03/2014 05:33 PM, Tejun Heo wrote:
>>> On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
>>>> rcu read lock should be held when calling and working with task_css_set.
>>>>
>>>> This patch also fixes a related lockdep warning.
>>>
>>> Hmmm... PF_EXITING should be visible at that point and cset can't
>>> change anymore.  We prolly need to update lockdep annotation rather
>>> than adding spurious rcu locking around it.  Against which branch is
>>> it?  Can you please post the lockdep warning?
>>
>> I see it on -next.
>
> I think the right thing to do is using task_css_set_check() with
> PF_EXITING check and add comment explaining that cset can no longer
> change.  Can you please write up a patch?

Hrm... there is a PF_EXITING check there already:

	#define task_css_set_check(task, __c)			\
		rcu_dereference_check((task)->cgroups,		\
		lockdep_is_held(&cgroup_mutex) ||		\
		lockdep_is_held(&css_set_rwsem) ||		\
		((task)->flags & PF_EXITING) || (__c))

I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.


Thanks,
Sasha

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

On 03/03/2014 06:00 PM, Sasha Levin wrote:
> On 03/03/2014 05:45 PM, Tejun Heo wrote:
>> On Mon, Mar 03, 2014 at 05:43:11PM -0500, Sasha Levin wrote:
>>> On 03/03/2014 05:33 PM, Tejun Heo wrote:
>>>> On Sat, Mar 01, 2014 at 10:00:11PM -0500, Sasha Levin wrote:
>>>>> rcu read lock should be held when calling and working with task_css_set.
>>>>>
>>>>> This patch also fixes a related lockdep warning.
>>>>
>>>> Hmmm... PF_EXITING should be visible at that point and cset can't
>>>> change anymore.  We prolly need to update lockdep annotation rather
>>>> than adding spurious rcu locking around it.  Against which branch is
>>>> it?  Can you please post the lockdep warning?
>>>
>>> I see it on -next.
>>
>> I think the right thing to do is using task_css_set_check() with
>> PF_EXITING check and add comment explaining that cset can no longer
>> change.  Can you please write up a patch?
>
> Hrm... there is a PF_EXITING check there already:
>
>      #define task_css_set_check(task, __c)            \
>          rcu_dereference_check((task)->cgroups,        \
>          lockdep_is_held(&cgroup_mutex) ||        \
>          lockdep_is_held(&css_set_rwsem) ||        \
>          ((task)->flags & PF_EXITING) || (__c))
>
> I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.

Hi Tejun,

It bisects down to your patch: "cgroup: drop task_lock() protection around task->cgroups". I'll
look into it later unless it's obvious to you.


Thanks,
Sasha

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Tejun Heo]

On Tue, Mar 04, 2014 at 12:20:45PM -0500, Sasha Levin wrote:
> >Hrm... there is a PF_EXITING check there already:
> >
> >     #define task_css_set_check(task, __c)            \
> >         rcu_dereference_check((task)->cgroups,        \
> >         lockdep_is_held(&cgroup_mutex) ||        \
> >         lockdep_is_held(&css_set_rwsem) ||        \
> >         ((task)->flags & PF_EXITING) || (__c))
> >
> >I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.
> 
> Hi Tejun,
> 
> It bisects down to your patch: "cgroup: drop task_lock() protection
> around task->cgroups". I'll look into it later unless it's obvious
> to you.

Hmmm... maybe I'm confused and PF_EXITING is not set there and
task_lock was what held off the lockdep warning.  Confused....

Thanks.

-- 
tejun

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Li Zefan]

On 2014/3/5 3:47, Tejun Heo wrote:
> On Tue, Mar 04, 2014 at 12:20:45PM -0500, Sasha Levin wrote:
>>> Hrm... there is a PF_EXITING check there already:
>>>
>>>     #define task_css_set_check(task, __c)            \
>>>         rcu_dereference_check((task)->cgroups,        \
>>>         lockdep_is_held(&cgroup_mutex) ||        \
>>>         lockdep_is_held(&css_set_rwsem) ||        \
>>>         ((task)->flags & PF_EXITING) || (__c))
>>>
>>> I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.
>>
>> Hi Tejun,
>>
>> It bisects down to your patch: "cgroup: drop task_lock() protection
>> around task->cgroups". I'll look into it later unless it's obvious
>> to you.
> 
> Hmmm... maybe I'm confused and PF_EXITING is not set there and
> task_lock was what held off the lockdep warning.  Confused....
> 

Because this cgroup_exit() is called in a failure path in copy_process().

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Sasha Levin]

On 03/04/2014 07:57 PM, Li Zefan wrote:
> On 2014/3/5 3:47, Tejun Heo wrote:
>> On Tue, Mar 04, 2014 at 12:20:45PM -0500, Sasha Levin wrote:
>>>> Hrm... there is a PF_EXITING check there already:
>>>>
>>>>      #define task_css_set_check(task, __c)            \
>>>>          rcu_dereference_check((task)->cgroups,        \
>>>>          lockdep_is_held(&cgroup_mutex) ||        \
>>>>          lockdep_is_held(&css_set_rwsem) ||        \
>>>>          ((task)->flags & PF_EXITING) || (__c))
>>>>
>>>> I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.
>>>
>>> Hi Tejun,
>>>
>>> It bisects down to your patch: "cgroup: drop task_lock() protection
>>> around task->cgroups". I'll look into it later unless it's obvious
>>> to you.
>>
>> Hmmm... maybe I'm confused and PF_EXITING is not set there and
>> task_lock was what held off the lockdep warning.  Confused....
>>
>
> Because this cgroup_exit() is called in a failure path in copy_process().

It seems there was no conclusion here and it still happens in -next, anything
we can do about it?

Thanks,
Sasha

Re: [PATCH] cgroup: missing rcu read lock around task_css_set

[Li Zefan]

On 2014/3/27 23:35, Sasha Levin wrote:
> On 03/04/2014 07:57 PM, Li Zefan wrote:
>> On 2014/3/5 3:47, Tejun Heo wrote:
>>> On Tue, Mar 04, 2014 at 12:20:45PM -0500, Sasha Levin wrote:
>>>>> Hrm... there is a PF_EXITING check there already:
>>>>>
>>>>>      #define task_css_set_check(task, __c)            \
>>>>>          rcu_dereference_check((task)->cgroups,        \
>>>>>          lockdep_is_held(&cgroup_mutex) ||        \
>>>>>          lockdep_is_held(&css_set_rwsem) ||        \
>>>>>          ((task)->flags & PF_EXITING) || (__c))
>>>>>
>>>>> I see it's not happening on Linus's master so I'll run a bisection to figure out what broke it.
>>>>
>>>> Hi Tejun,
>>>>
>>>> It bisects down to your patch: "cgroup: drop task_lock() protection
>>>> around task->cgroups". I'll look into it later unless it's obvious
>>>> to you.
>>>
>>> Hmmm... maybe I'm confused and PF_EXITING is not set there and
>>> task_lock was what held off the lockdep warning.  Confused....
>>>
>>
>> Because this cgroup_exit() is called in a failure path in copy_process().
> 
> It seems there was no conclusion here and it still happens in -next, anything
> we can do about it?
> 

I'll send a patch to fix it. Thanks!