cgroup: net_cls: fix false-positive "suspicious RCU usage"

PATCH Superseded Konstantin Khlebnikov 3 participants 7 messages Jul 21, 2015 – Jul 25, 2015

Subsystems (3)

Networking Network drivers cgroup

lore

Share & tools

lore

Konstantin Khlebnikov

author

2015-07-21 16:46 UTC #0 lore raw

PATCH 1/1 cgroup: net_cls: fix false-positive "suspicious RCU usage"

In dev_queue_xmit() net_cls protected with rcu-bh.

[  270.730026] ===============================
[  270.730029] [ INFO: suspicious RCU usage. ]
[  270.730033] 4.2.0-rc3+ #2 Not tainted
[  270.730036] -------------------------------
[  270.730040] include/linux/cgroup.h:353 suspicious rcu_dereference_check() usage!
[  270.730041] other info that might help us debug this:
[  270.730043] rcu_scheduler_active = 1, debug_locks = 1
[  270.730045] 2 locks held by dhclient/748:
[  270.730046]  #0:  (rcu_read_lock_bh){......}, at: [<ffffffff81682b70>] __dev_queue_xmit+0x50/0x960
[  270.730085]  #1:  (&qdisc_tx_lock){+.....}, at: [<ffffffff81682d60>] __dev_queue_xmit+0x240/0x960
[  270.730090] stack backtrace:
[  270.730096] CPU: 0 PID: 748 Comm: dhclient Not tainted 4.2.0-rc3+ #2
[  270.730098] Hardware name: OpenStack Foundation OpenStack Nova, BIOS Bochs 01/01/2011
[  270.730100]  0000000000000001 ffff8800bafeba58 ffffffff817ad487 0000000000000007
[  270.730103]  ffff880232a0a780 ffff8800bafeba88 ffffffff810ca4f2 ffff88022fb23e00
[  270.730105]  ffff880232a0a780 ffff8800bafebb68 ffff8800bafebb68 ffff8800bafebaa8
[  270.730108] Call Trace:
[  270.730121]  [<ffffffff817ad487>] dump_stack+0x4c/0x65
[  270.730148]  [<ffffffff810ca4f2>] lockdep_rcu_suspicious+0xe2/0x120
[  270.730153]  [<ffffffff816a62d2>] task_cls_state+0x92/0xa0
[  270.730158]  [<ffffffffa00b534f>] cls_cgroup_classify+0x4f/0x120 [cls_cgroup]
[  270.730164]  [<ffffffff816aac74>] tc_classify_compat+0x74/0xc0
[  270.730166]  [<ffffffff816ab573>] tc_classify+0x33/0x90
[  270.730170]  [<ffffffffa00bcb0a>] htb_enqueue+0xaa/0x4a0 [sch_htb]
[  270.730172]  [<ffffffff81682e26>] __dev_queue_xmit+0x306/0x960
[  270.730174]  [<ffffffff81682b70>] ? __dev_queue_xmit+0x50/0x960
[  270.730176]  [<ffffffff816834a3>] dev_queue_xmit_sk+0x13/0x20
[  270.730185]  [<ffffffff81787770>] dev_queue_xmit+0x10/0x20
[  270.730187]  [<ffffffff8178b91c>] packet_snd.isra.62+0x54c/0x760
[  270.730190]  [<ffffffff8178be25>] packet_sendmsg+0x2f5/0x3f0
[  270.730203]  [<ffffffff81665245>] ? sock_def_readable+0x5/0x190
[  270.730210]  [<ffffffff817b64bb>] ? _raw_spin_unlock+0x2b/0x40
[  270.730216]  [<ffffffff8173bcbc>] ? unix_dgram_sendmsg+0x5cc/0x640
[  270.730219]  [<ffffffff8165f367>] sock_sendmsg+0x47/0x50
[  270.730221]  [<ffffffff8165f42f>] sock_write_iter+0x7f/0xd0
[  270.730232]  [<ffffffff811fd4c7>] __vfs_write+0xa7/0xf0
[  270.730234]  [<ffffffff811fe5b8>] vfs_write+0xb8/0x190
[  270.730236]  [<ffffffff811fe8c2>] SyS_write+0x52/0xb0
[  270.730239]  [<ffffffff817b6bae>] entry_SYSCALL_64_fastpath+0x12/0x76

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 net/core/netclassid_cgroup.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/core/netclassid_cgroup.c +2 -1

index 1f2a126f4ffa..515939034298 100644
@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state
 
 struct cgroup_cls_state *task_cls_state(struct task_struct *p)
 {
-	return css_cls_state(task_css(p, net_cls_cgrp_id));
+       return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+			       rcu_read_lock_bh_held()));
 }
 EXPORT_SYMBOL_GPL(task_cls_state);

David Miller

maintainer

2015-07-22 05:28 UTC #1 lore raw

From: Konstantin Khlebnikov <[email protected]>
Date: Tue, 21 Jul 2015 19:46:29 +0300

net/core/netclassid_cgroup.c static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state · line 27

Quoted 7 lines

net/core/netclassid_cgroup.c +2 -1

@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state
 
 struct cgroup_cls_state *task_cls_state(struct task_struct *p)
 {
-	return css_cls_state(task_css(p, net_cls_cgrp_id));
+       return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+			       rcu_read_lock_bh_held()));

You've made a serious mess of the indentation here.

First of all, you've changed the correct plain "TAB" before the 'return' line into a TAB and two SPACE characters.

Secondly, the second line needs to be precisely indented to the exact column following the openning parenthesis of the task_css_check() call on the previous line.

Konstantin Khlebnikov

author

2015-07-22 09:23 UTC #2 lore raw

PATCH v2 1/1 cgroup: net_cls: fix false-positive "suspicious RCU usage"

In reply to Konstantin Khlebnikov

In dev_queue_xmit() net_cls protected with rcu-bh.

[  270.730026] ===============================
[  270.730029] [ INFO: suspicious RCU usage. ]
[  270.730033] 4.2.0-rc3+ #2 Not tainted
[  270.730036] -------------------------------
[  270.730040] include/linux/cgroup.h:353 suspicious rcu_dereference_check() usage!
[  270.730041] other info that might help us debug this:
[  270.730043] rcu_scheduler_active = 1, debug_locks = 1
[  270.730045] 2 locks held by dhclient/748:
[  270.730046]  #0:  (rcu_read_lock_bh){......}, at: [<ffffffff81682b70>] __dev_queue_xmit+0x50/0x960
[  270.730085]  #1:  (&qdisc_tx_lock){+.....}, at: [<ffffffff81682d60>] __dev_queue_xmit+0x240/0x960
[  270.730090] stack backtrace:
[  270.730096] CPU: 0 PID: 748 Comm: dhclient Not tainted 4.2.0-rc3+ #2
[  270.730098] Hardware name: OpenStack Foundation OpenStack Nova, BIOS Bochs 01/01/2011
[  270.730100]  0000000000000001 ffff8800bafeba58 ffffffff817ad487 0000000000000007
[  270.730103]  ffff880232a0a780 ffff8800bafeba88 ffffffff810ca4f2 ffff88022fb23e00
[  270.730105]  ffff880232a0a780 ffff8800bafebb68 ffff8800bafebb68 ffff8800bafebaa8
[  270.730108] Call Trace:
[  270.730121]  [<ffffffff817ad487>] dump_stack+0x4c/0x65
[  270.730148]  [<ffffffff810ca4f2>] lockdep_rcu_suspicious+0xe2/0x120
[  270.730153]  [<ffffffff816a62d2>] task_cls_state+0x92/0xa0
[  270.730158]  [<ffffffffa00b534f>] cls_cgroup_classify+0x4f/0x120 [cls_cgroup]
[  270.730164]  [<ffffffff816aac74>] tc_classify_compat+0x74/0xc0
[  270.730166]  [<ffffffff816ab573>] tc_classify+0x33/0x90
[  270.730170]  [<ffffffffa00bcb0a>] htb_enqueue+0xaa/0x4a0 [sch_htb]
[  270.730172]  [<ffffffff81682e26>] __dev_queue_xmit+0x306/0x960
[  270.730174]  [<ffffffff81682b70>] ? __dev_queue_xmit+0x50/0x960
[  270.730176]  [<ffffffff816834a3>] dev_queue_xmit_sk+0x13/0x20
[  270.730185]  [<ffffffff81787770>] dev_queue_xmit+0x10/0x20
[  270.730187]  [<ffffffff8178b91c>] packet_snd.isra.62+0x54c/0x760
[  270.730190]  [<ffffffff8178be25>] packet_sendmsg+0x2f5/0x3f0
[  270.730203]  [<ffffffff81665245>] ? sock_def_readable+0x5/0x190
[  270.730210]  [<ffffffff817b64bb>] ? _raw_spin_unlock+0x2b/0x40
[  270.730216]  [<ffffffff8173bcbc>] ? unix_dgram_sendmsg+0x5cc/0x640
[  270.730219]  [<ffffffff8165f367>] sock_sendmsg+0x47/0x50
[  270.730221]  [<ffffffff8165f42f>] sock_write_iter+0x7f/0xd0
[  270.730232]  [<ffffffff811fd4c7>] __vfs_write+0xa7/0xf0
[  270.730234]  [<ffffffff811fe5b8>] vfs_write+0xb8/0x190
[  270.730236]  [<ffffffff811fe8c2>] SyS_write+0x52/0xb0
[  270.730239]  [<ffffffff817b6bae>] entry_SYSCALL_64_fastpath+0x12/0x76

Signed-off-by Konstantin Khlebnikov <[email protected]>

---
 net/core/netclassid_cgroup.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/core/netclassid_cgroup.c +2 -1

index 1f2a126f4ffa..6441f47b1a8f 100644
@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state
 
 struct cgroup_cls_state *task_cls_state(struct task_struct *p)
 {
-	return css_cls_state(task_css(p, net_cls_cgrp_id));
+	return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+					    rcu_read_lock_bh_held()));
 }
 EXPORT_SYMBOL_GPL(task_cls_state);

Daniel Borkmann

2015-07-22 11:56 UTC #3 lore raw

Quoted 1 line · Konstantin Khlebnikov

In dev_queue_xmit() net_cls protected with rcu-bh.

...

Quoted 10 lines

Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  net/core/netclassid_cgroup.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
net/core/netclassid_cgroup.c +0 -0
index 1f2a126f4ffa..6441f47b1a8f 100644

net/core/netclassid_cgroup.c static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state · line 27

Quoted 6 lines

net/core/netclassid_cgroup.c +2 -1

@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state
 
  struct cgroup_cls_state *task_cls_state(struct task_struct *p)
  {
-	return css_cls_state(task_css(p, net_cls_cgrp_id));
+	return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+					    rcu_read_lock_bh_held()));

Did you also check that after your patch this doesn't trigger on ingress either? There, this code path could be invoked under rcu_read_lock(). So, perhaps you need to check for both.

Quoted 2 lines

net/core/netclassid_cgroup.c +0 -0
  }
  EXPORT_SYMBOL_GPL(task_cls_state);

Konstantin Khlebnikov

author

2015-07-22 12:03 UTC #4 lore raw

Quoted 15 lines · Daniel Borkmann

Quoted 1 line · Konstantin Khlebnikov
In dev_queue_xmit() net_cls protected with rcu-bh.
...
Quoted 12 lines
Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  net/core/netclassid_cgroup.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
net/core/netclassid_cgroup.c +0 -0
index 1f2a126f4ffa..6441f47b1a8f 100644
@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state
 *css_cls_state(struct cgroup_subsys_state
 

Quoted 9 lines

net/core/netclassid_cgroup.c static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state · line 27
Quoted 5 lines
net/core/netclassid_cgroup.c +2 -1
  struct cgroup_cls_state *task_cls_state(struct task_struct *p)
  {
-    return css_cls_state(task_css(p, net_cls_cgrp_id));
+    return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+                        rcu_read_lock_bh_held()));
Did you also check that after your patch this doesn't trigger on ingress either? There, this code path could be invoked under rcu_read_lock(). So, perhaps you need to check for both.

I haven't seen warnings with this patch. rcu_read_lock_held() is checked inside rcu_dereference_check() inside task_css_check().

So, overall check expression is:

lockdep_is_held(&cgroup_mutex) || <- in task_css_set_check lockdep_is_held(&css_set_rwsem) ||
((task)->flags & PF_EXITING) || rcu_read_lock_bh_held() || <- mine rcu_read_lock_held() <- in rcu_dereference_check

Quoted 3 lines

Quoted 3 lines
net/core/netclassid_cgroup.c +0 -0
  }
  EXPORT_SYMBOL_GPL(task_cls_state);

Signature · 1 line

Konstantin

Daniel Borkmann

2015-07-22 12:06 UTC #5 lore raw

Quoted 26 lines · Konstantin Khlebnikov

Quoted 15 lines · Daniel Borkmann
Quoted 1 line · Konstantin Khlebnikov
In dev_queue_xmit() net_cls protected with rcu-bh.
...
Quoted 12 lines
Signed-off-by Konstantin Khlebnikov <[email protected]>
---
  net/core/netclassid_cgroup.c |    3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
net/core/netclassid_cgroup.c +0 -0
index 1f2a126f4ffa..6441f47b1a8f 100644
@@ -23,7 +23,8 @@ static inline struct cgroup_cls_state
 *css_cls_state(struct cgroup_subsys_state
 
Quoted 9 lines
net/core/netclassid_cgroup.c static inline struct cgroup_cls_state *css_cls_state(struct cgroup_subsys_state · line 27
Quoted 5 lines
net/core/netclassid_cgroup.c +2 -1
  struct cgroup_cls_state *task_cls_state(struct task_struct *p)
  {
-    return css_cls_state(task_css(p, net_cls_cgrp_id));
+    return css_cls_state(task_css_check(p, net_cls_cgrp_id,
+                        rcu_read_lock_bh_held()));
Did you also check that after your patch this doesn't trigger on ingress either? There, this code path could be invoked under rcu_read_lock(). So, perhaps you need to check for both.

Quoted 2 lines

I haven't seen warnings with this patch. rcu_read_lock_held() is checked inside rcu_dereference_check() inside task_css_check().

Thanks, agreed.

David Miller

maintainer Applied

2015-07-25 07:00 UTC #6 lore raw

In reply to Konstantin Khlebnikov

From: Konstantin Khlebnikov <[email protected]>
Date: Wed, 22 Jul 2015 12:23:20 +0300

Quoted 1 line

In dev_queue_xmit() net_cls protected with rcu-bh.

...

Quoted 1 line

Signed-off-by Konstantin Khlebnikov <[email protected]>

Applied, thanks.