mm/mempolicy: fix sleeping function called from invalid context

PATCH Superseded Gu Zheng 6 participants 15 messages Jun 5, 2014 – Jun 25, 2014

Subsystems (4)

Stable branch Memory management cgroup MM NUMA placement

lore

Share & tools

lore

Gu Zheng

author

2014-06-05 08:28 UTC #0 lore raw

PATCH 1/1 mm/mempolicy: fix sleeping function called from invalid context

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

The cause is that cpuset_mems_allowed() try to take mutex_lock(&callback_mutex) under the rcu_read_lock(which was hold in __mpol_dup()). And in cpuset_mems_allowed(), the access to cpuset is under rcu_read_lock, so in __mpol_dup, we can reduce the rcu_read_lock protection region to protect the access to cpuset only in current_cpuset_is_being_rebound(). So that we can avoid this bug.

Signed-off-by Gu Zheng <[email protected]>

---
 kernel/cpuset.c |    8 +++++++-
 mm/mempolicy.c  |    2 --
 2 files changed, 7 insertions(+), 3 deletions(-)

kernel/cpuset.c +7 -1

index 3d54c41..a735402 100644
@@ -1188,7 +1188,13 @@ done:
 
 int current_cpuset_is_being_rebound(void)
 {
-	return task_cs(current) == cpuset_being_rebound;
+	int ret;
+
+	rcu_read_lock();
+	ret = task_cs(current) == cpuset_being_rebound;
+	rcu_read_unlock();
+
+	return ret;
 }
 
 static int update_relax_domain_level(struct cpuset *cs, s64 val)

mm/mempolicy.c +0 -2

index 78e1472..b58a9d5 100644
@@ -2138,7 +2138,6 @@ struct mempolicy *__mpol_dup(struct mempolicy *old)
 	} else
 		*new = *old;
 
-	rcu_read_lock();
 	if (current_cpuset_is_being_rebound()) {
 		nodemask_t mems = cpuset_mems_allowed(current);
 		if (new->flags & MPOL_F_REBINDING)

@@ -2146,7 +2145,6 @@ struct mempolicy *__mpol_dup(struct mempolicy *old)
 		else
 			mpol_rebind_policy(new, &mems, MPOL_REBIND_ONCE);
 	}
-	rcu_read_unlock();
 	atomic_set(&new->refcnt, 1);
 	return new;
 }

Signature · 7 lines

1.7.7

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Greg KH

2014-06-05 14:18 UTC #1 lore raw

Quoted 34 lines · Gu Zheng

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

Quoted 5 lines

Signed-off-by Gu Zheng <[email protected]>

---
 kernel/cpuset.c |    8 +++++++-
 mm/mempolicy.c  |    2 --
 2 files changed, 7 insertions(+), 3 deletions(-)

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.

</formletter>

Gu Zheng

author

2014-06-06 09:34 UTC #2 lore raw

Hi Greg,

Quoted 41 lines · Greg KH

Quoted 34 lines · Gu Zheng

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

Quoted 5 lines

Signed-off-by Gu Zheng <[email protected]>

---
 kernel/cpuset.c |    8 +++++++-
 mm/mempolicy.c  |    2 --
 2 files changed, 7 insertions(+), 3 deletions(-)

Quoted 8 lines

<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.
</formletter>

Thanks for your reminder.
I'll follow the right rules.

Regards,
Gu

Quoted 8 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected] For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a> .

Andrew Morton

maintainer

2014-06-05 20:23 UTC #3 lore raw

In reply to Gu Zheng

Quoted 41 lines · On Thu, 5 Jun 2014 16:28:52 +0800 Gu Zheng <[email protected]> wrote:

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

...

kernel/cpuset.c +0 -0

@@ -1188,7 +1188,13 @@ done:
 
 int current_cpuset_is_being_rebound(void)

kernel/cpuset.c done: · line 1198

Quoted 10 lines

kernel/cpuset.c +7 -1

 {
-	return task_cs(current) == cpuset_being_rebound;
+	int ret;
+
+	rcu_read_lock();
+	ret = task_cs(current) == cpuset_being_rebound;
+	rcu_read_unlock();
+
+	return ret;
 }

Looks fishy to me. If the rcu_read_lock() stabilizes cpuset_being_rebound then cpuset_being_rebound can change immediately after rcu_read_unlock() and `ret' is now wrong.

Anyway. Tejun, this one is yours please ;)

Gu Zheng

author

2014-06-06 10:07 UTC #4 lore raw

(cc'ing Li)

Hi Andrew,
Thanks for your comment.

Quoted 42 lines · Andrew Morton

Quoted 40 lines · On Thu, 5 Jun 2014 16:28:52 +0800 Gu Zheng <[email protected]> wrote:

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

...

kernel/cpuset.c +0 -0

@@ -1188,7 +1188,13 @@ done:

Quoted 15 lines

kernel/cpuset.c done: · line 1198
Quoted 11 lines
kernel/cpuset.c +7 -1
 int current_cpuset_is_being_rebound(void)
 {
-	return task_cs(current) == cpuset_being_rebound;
+	int ret;
+
+	rcu_read_lock();
+	ret = task_cs(current) == cpuset_being_rebound;
+	rcu_read_unlock();
+
+	return ret;
 }
Looks fishy to me. If the rcu_read_lock() stabilizes cpuset_being_rebound then cpuset_being_rebound can change immediately after rcu_read_unlock() and `ret' is now wrong.

IMO, whether cpuset_being_rebound changed or not is immaterial here, we just want to know whether the cpuset is being rebound at that point.

Quoted 2 lines

Anyway. Tejun, this one is yours please ;)

To Tejun, Li:
Any comment? And if I misread something, please correct me?

Thanks,
Gu

Quoted 2 lines

.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

David Rientjes

2014-06-08 22:47 UTC #5 lore raw

Quoted 56 lines · Gu Zheng

Quoted 40 lines

When running with the kernel(3.15-rc7+), the follow bug occurs:

[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b

...

kernel/cpuset.c +0 -0

@@ -1188,7 +1188,13 @@ done:

Quoted 15 lines

kernel/cpuset.c done: · line 1198
Quoted 11 lines
kernel/cpuset.c +7 -1
 int current_cpuset_is_being_rebound(void)
 {
-	return task_cs(current) == cpuset_being_rebound;
+	int ret;
+
+	rcu_read_lock();
+	ret = task_cs(current) == cpuset_being_rebound;
+	rcu_read_unlock();
+
+	return ret;
 }
Looks fishy to me. If the rcu_read_lock() stabilizes cpuset_being_rebound then cpuset_being_rebound can change immediately after rcu_read_unlock() and `ret' is now wrong.

Quoted 3 lines

IMO, whether cpuset_being_rebound changed or not is immaterial here, we just want to know whether the cpuset is being rebound at that point.

I think your patch addresses the problem that you're reporting but misses the larger problem with cpuset.mems rebinding on fork(). When the forker's task_struct is duplicated (which includes ->mems_allowed) and it races with an update to cpuset_being_rebound in update_tasks_nodemask() then the task's mems_allowed doesn't get updated.

Gu Zheng

author

2014-06-09 08:48 UTC #6 lore raw

Hi David,

Quoted 62 lines · David Rientjes

Quoted 56 lines · Gu Zheng
Quoted 40 lines
Quoted 40 lines
When running with the kernel(3.15-rc7+), the follow bug occurs:
[ 9969.258987] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[ 9969.359906] in_atomic(): 1, irqs_disabled(): 0, pid: 160655, name: python
[ 9969.441175] INFO: lockdep is turned off.
[ 9969.488184] CPU: 26 PID: 160655 Comm: python Tainted: G       A      3.15.0-rc7+ #85
[ 9969.581032] Hardware name: FUJITSU-SV PRIMEQUEST 1800E/SB, BIOS PRIMEQUEST 1000 Series BIOS Version 1.39 11/16/2012
[ 9969.706052]  ffffffff81a20e60 ffff8803e941fbd0 ffffffff8162f523 ffff8803e941fd18
[ 9969.795323]  ffff8803e941fbe0 ffffffff8109995a ffff8803e941fc58 ffffffff81633e6c
[ 9969.884710]  ffffffff811ba5dc ffff880405c6b480 ffff88041fdd90a0 0000000000002000
[ 9969.974071] Call Trace:
[ 9970.003403]  [<ffffffff8162f523>] dump_stack+0x4d/0x66
[ 9970.065074]  [<ffffffff8109995a>] __might_sleep+0xfa/0x130
[ 9970.130743]  [<ffffffff81633e6c>] mutex_lock_nested+0x3c/0x4f0
[ 9970.200638]  [<ffffffff811ba5dc>] ? kmem_cache_alloc+0x1bc/0x210
[ 9970.272610]  [<ffffffff81105807>] cpuset_mems_allowed+0x27/0x140
[ 9970.344584]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.409282]  [<ffffffff811b1385>] __mpol_dup+0xe5/0x150
[ 9970.471897]  [<ffffffff811b1303>] ? __mpol_dup+0x63/0x150
[ 9970.536585]  [<ffffffff81068c86>] ? copy_process.part.23+0x606/0x1d40
[ 9970.613763]  [<ffffffff810bf28d>] ? trace_hardirqs_on+0xd/0x10
[ 9970.683660]  [<ffffffff810ddddf>] ? monotonic_to_bootbased+0x2f/0x50
[ 9970.759795]  [<ffffffff81068cf0>] copy_process.part.23+0x670/0x1d40
[ 9970.834885]  [<ffffffff8106a598>] do_fork+0xd8/0x380
[ 9970.894375]  [<ffffffff81110e4c>] ? __audit_syscall_entry+0x9c/0xf0
[ 9970.969470]  [<ffffffff8106a8c6>] SyS_clone+0x16/0x20
[ 9971.030011]  [<ffffffff81642009>] stub_clone+0x69/0x90
[ 9971.091573]  [<ffffffff81641c29>] ? system_call_fastpath+0x16/0x1b
The cause is that cpuset_mems_allowed() try to take mutex_lock(&callback_mutex) under the rcu_read_lock(which was hold in __mpol_dup()). And in cpuset_mems_allowed(), the access to cpuset is under rcu_read_lock, so in __mpol_dup, we can reduce the rcu_read_lock protection region to protect the access to cpuset only in current_cpuset_is_being_rebound(). So that we can avoid this bug.
...
kernel/cpuset.c +0 -0
@@ -1188,7 +1188,13 @@ done:
 
Quoted 15 lines
kernel/cpuset.c done: · line 1198
Quoted 11 lines
kernel/cpuset.c +7 -1
 int current_cpuset_is_being_rebound(void)
 {
-	return task_cs(current) == cpuset_being_rebound;
+	int ret;
+
+	rcu_read_lock();
+	ret = task_cs(current) == cpuset_being_rebound;
+	rcu_read_unlock();
+
+	return ret;
 }
Looks fishy to me. If the rcu_read_lock() stabilizes cpuset_being_rebound then cpuset_being_rebound can change immediately after rcu_read_unlock() and `ret' is now wrong.
Quoted 3 lines
IMO, whether cpuset_being_rebound changed or not is immaterial here, we just want to know whether the cpuset is being rebound at that point.

Quoted 5 lines

I think your patch addresses the problem that you're reporting but misses the larger problem with cpuset.mems rebinding on fork(). When the forker's task_struct is duplicated (which includes ->mems_allowed) and it races with an update to cpuset_being_rebound in update_tasks_nodemask() then the task's mems_allowed doesn't get updated.

Yes, you are right, this patch just wants to address the bug reported above. The race condition you mentioned above inherently exists there, but it is yet another issue, the rcu lock here makes no sense to it, and I think we need additional sync-mechanisms if want to fix it.
But thinking more, though the current implementation has flaw, but I worry about the negative effect if we really want to fix it. Or maybe the fear is unnecessary.:)

Thanks,
Gu

Quoted 2 lines

.

David Rientjes

2014-06-09 09:13 UTC #7 lore raw

Quoted 10 lines · Gu Zheng

Quoted 5 lines
I think your patch addresses the problem that you're reporting but misses the larger problem with cpuset.mems rebinding on fork(). When the forker's task_struct is duplicated (which includes ->mems_allowed) and it races with an update to cpuset_being_rebound in update_tasks_nodemask() then the task's mems_allowed doesn't get updated.
Yes, you are right, this patch just wants to address the bug reported above. The race condition you mentioned above inherently exists there, but it is yet another issue, the rcu lock here makes no sense to it, and I think we need additional sync-mechanisms if want to fix it.

Yes, the rcu lock is not providing protection for any critical section here that requires (1) the forker's cpuset to be stored in cpuset_being_rebound or (2) the forked thread's cpuset to be rebound by the cpuset nodemask update, and no race involving the two.

Quoted 4 lines

But thinking more, though the current implementation has flaw, but I worry about the negative effect if we really want to fix it. Or maybe the fear is unnecessary.:)

It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?

Gu Zheng

author

2014-06-09 09:58 UTC #8 lore raw

Hi David,

Quoted 18 lines · David Rientjes

Quoted 10 lines · Gu Zheng
Quoted 5 lines
I think your patch addresses the problem that you're reporting but misses the larger problem with cpuset.mems rebinding on fork(). When the forker's task_struct is duplicated (which includes ->mems_allowed) and it races with an update to cpuset_being_rebound in update_tasks_nodemask() then the task's mems_allowed doesn't get updated.
Yes, you are right, this patch just wants to address the bug reported above. The race condition you mentioned above inherently exists there, but it is yet another issue, the rcu lock here makes no sense to it, and I think we need additional sync-mechanisms if want to fix it.
Yes, the rcu lock is not providing protection for any critical section here that requires (1) the forker's cpuset to be stored in cpuset_being_rebound or (2) the forked thread's cpuset to be rebound by the cpuset nodemask update, and no race involving the two.

Quoted 8 lines

Quoted 4 lines
But thinking more, though the current implementation has flaw, but I worry about the negative effect if we really want to fix it. Or maybe the fear is unnecessary.:)
It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?

To be honest, I'm busy with other schedule. And if you(or other guys) have proper proposal, please go ahead.

To Tejun, Li and Andrew:
Any comment? Or could you apply this *bug fix* first?

Regards,
Gu

Li Zefan

2014-06-10 02:58 UTC #9 lore raw

In reply to David Rientjes

Quoted 13 lines · David Rientjes

Quoted 10 lines · Gu Zheng
Quoted 5 lines
I think your patch addresses the problem that you're reporting but misses the larger problem with cpuset.mems rebinding on fork(). When the forker's task_struct is duplicated (which includes ->mems_allowed) and it races with an update to cpuset_being_rebound in update_tasks_nodemask() then the task's mems_allowed doesn't get updated.
Yes, you are right, this patch just wants to address the bug reported above. The race condition you mentioned above inherently exists there, but it is yet another issue, the rcu lock here makes no sense to it, and I think we need additional sync-mechanisms if want to fix it.

Quoted 5 lines

Yes, the rcu lock is not providing protection for any critical section here that requires (1) the forker's cpuset to be stored in cpuset_being_rebound or (2) the forked thread's cpuset to be rebound by the cpuset nodemask update, and no race involving the two.

Yes, this is a long-standing issue. Besides the race you described, the child task's mems_allowed can be wrong if the cpuset's nodemask changes before the child has been added to the cgroup's tasklist.

I remember Tejun once said he wanted to disallow task migration between cgroups during fork, and that should fix this problem.

Quoted 8 lines

Quoted 4 lines
But thinking more, though the current implementation has flaw, but I worry about the negative effect if we really want to fix it. Or maybe the fear is unnecessary.:)
It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?

Sounds you have other idea?

David Rientjes

2014-06-10 22:16 UTC #10 lore raw

Quoted 6 lines · Li Zefan

Quoted 5 lines
Yes, the rcu lock is not providing protection for any critical section here that requires (1) the forker's cpuset to be stored in cpuset_being_rebound or (2) the forked thread's cpuset to be rebound by the cpuset nodemask update, and no race involving the two.

Quoted 7 lines

Yes, this is a long-standing issue. Besides the race you described, the child task's mems_allowed can be wrong if the cpuset's nodemask changes before the child has been added to the cgroup's tasklist.
I remember Tejun once said he wanted to disallow task migration between cgroups during fork, and that should fix this problem.

Ok, I don't want to fix it in cpusets if cgroups will eventually prevent it, so I need an understanding of the long term plan. Will cgroups continue to allow migration during fork(), Tejun?

Quoted 6 lines

Quoted 3 lines
It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?
Sounds you have other idea?

It wouldn't be too difficult with a cgroup post fork callback into the cpuset code to rebind the nodemask if it has changed, but with my above concern those might be yanked out eventually :)

Tejun Heo

maintainer

2014-06-20 21:01 UTC #11 lore raw

In reply to Li Zefan

Hello, Li.

Sorry about the long delay.

Quoted 6 lines · Li Zefan

Yes, this is a long-standing issue. Besides the race you described, the child task's mems_allowed can be wrong if the cpuset's nodemask changes before the child has been added to the cgroup's tasklist.
I remember Tejun once said he wanted to disallow task migration between cgroups during fork, and that should fix this problem.

I'm having trouble remembering but yeah enforcing stricter behavior across fork could be beneficial. Hmmm... the problem with making forks exclusive against migrations is that we'll end up adding more locking to the fork path which isn't too nice.

Hmmm... other controllers (cgroup_freezer) can reliably synchronize the child's state to the cgroup it belongs to. Why can't cpuset? Is there something fundamentally missing in the cgroup API?

Quoted 5 lines

Quoted 3 lines
It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?
Sounds you have other idea?

I don't think the suggested patch breaks anything more than it was broken before and we should probably apply it for the time being. Li?

Thanks.

Signature · 7 lines

tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Li Zefan

2014-06-24 02:28 UTC #12 lore raw

Quoted 12 lines · Tejun Heo

Hello, Li.
Sorry about the long delay.
Quoted 6 lines · Li Zefan
Yes, this is a long-standing issue. Besides the race you described, the child task's mems_allowed can be wrong if the cpuset's nodemask changes before the child has been added to the cgroup's tasklist.
I remember Tejun once said he wanted to disallow task migration between cgroups during fork, and that should fix this problem.

Quoted 9 lines

I'm having trouble remembering but yeah enforcing stricter behavior across fork could be beneficial. Hmmm... the problem with making forks exclusive against migrations is that we'll end up adding more locking to the fork path which isn't too nice.
Hmmm... other controllers (cgroup_freezer) can reliably synchronize the child's state to the cgroup it belongs to. Why can't cpuset? Is there something fundamentally missing in the cgroup API?

cgroup_freezer uses the fork callback. We can also do this for cpuset as suggested by David, which adds a little bit overhead to the fork path.

David, care to send out a patch?

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
It needs to be slightly rewritten to work properly without negatively impacting the latency of fork(). Do you have the cycles to do it?
Sounds you have other idea?
I don't think the suggested patch breaks anything more than it was broken before and we should probably apply it for the time being. Li?

Yeah, we should apply Gu Zheng's patch any way.

Tejun Heo

maintainer

2014-06-24 20:58 UTC #13 lore raw

Hello,

Quoted 4 lines · Li Zefan

Quoted 2 lines
I don't think the suggested patch breaks anything more than it was broken before and we should probably apply it for the time being. Li?
Yeah, we should apply Gu Zheng's patch any way.

Gu Zheng, can you please respin the patch with updated explanation on the temporary nature of the change. I'll apply it once Li acks it.

Thanks.

Signature · 7 lines

tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Gu Zheng

author

2014-06-25 00:57 UTC #14 lore raw

Hi Tejun,

Quoted 10 lines · Tejun Heo

Hello,
Quoted 4 lines · Li Zefan
Quoted 2 lines
I don't think the suggested patch breaks anything more than it was broken before and we should probably apply it for the time being. Li?
Yeah, we should apply Gu Zheng's patch any way.
Gu Zheng, can you please respin the patch with updated explanation on the temporary nature of the change. I'll apply it once Li acks it.

OK, I'll resend it soon.

Regards,
Gu

Quoted 3 lines

Thanks.