DEBUG_RCU_HEAD: Debug and fix racy call_rcu() users

Poisoning the rcu_head callback list. Only for rcu tree for now.

Helps finding racy users of call_rcu(), which results in hangs because list entries are overwritten and/or skipped. Using the lower bit to poison because include/net/dst.h __pad_to_align_refcnt complains when struct rcu_head grows.

Quoted 6 lines · Mathieu Desnoyers

Poisoning the rcu_head callback list. Only for rcu tree for now.

Helps finding racy users of call_rcu(), which results in hangs because list entries are overwritten and/or skipped. Using the lower bit to poison because include/net/dst.h __pad_to_align_refcnt complains when struct rcu_head grows.

I see :)

include/linux/rcupdate.h line 49

Quoted 8 lines

include/linux/rcupdate.h +2 -0

@@ -45,6 +45,8 @@
  * struct rcu_head - callback structure for use with RCU
  * @next: next update requests in a list
  * @func: actual update function to call after the grace period.
+ *
+ * Debug mode assumes func pointer value is word-aligned.

Ouch, I guess you never tried CONFIG_CC_OPTIMIZE_FOR_SIZE=y ?

random extract from "nm -v vmlinux"

c0415e58 T in4_pton
c0415f66 T in6_pton
c04161f0 T in_aton
c0416240 T net_ratelimit
c0416250 t linkwatch_add_event
c0416281 t linkwatch_schedule_work
c0416301 T linkwatch_fire_event
c0416368 t __linkwatch_run_queue
c04164d2 t linkwatch_event
c04164f4 T linkwatch_run_queue
c0416500 T sk_chk_filter
c0416703 t sk_filter_rcu_release
c041671d T sk_detach_filter
c041676a T sk_attach_filter
c0416862 T sk_run_filter
c0416f5d T sk_filter
c0416fc4 t __flow_cache_shrink

gcc -v
Using built-in specs.
Target: i686-pc-linux-gnu
Configured with: ../gcc-4.4.1/configure --enable-languages=c,c++ --prefix=/usr Thread model: posix
gcc version 4.4.1 (GCC)

Quoted 3 lines · * Eric Dumazet ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Poisoning the rcu_head callback list. Only for rcu tree for now.

Quoted 6 lines

Quoted 4 lines

Helps finding racy users of call_rcu(), which results in hangs because list entries are overwritten and/or skipped. Using the lower bit to poison because include/net/dst.h __pad_to_align_refcnt complains when struct rcu_head grows.

I see :)

I don't know if there is an easy fix for __pad_to_align_refcnt ?

Quoted 11 lines · Mathieu Desnoyers

Quoted 9 lines · * Eric Dumazet ([email protected]) wrote:

Quoted 6 lines · Mathieu Desnoyers

Poisoning the rcu_head callback list. Only for rcu tree for now.

Helps finding racy users of call_rcu(), which results in hangs because list entries are overwritten and/or skipped. Using the lower bit to poison because include/net/dst.h __pad_to_align_refcnt complains when struct rcu_head grows.

I see :)

Quoted 1 line

I don't know if there is an easy fix for __pad_to_align_refcnt ?

This check was added to make sure some tbench regression was not added if dst->__refcnt was moved around, I am sure you dont care about tbench being 10% slower, do you ?

include/net/dst.h +2 -0

index 5a900dd..b8fba74 100644
@@ -154,7 +154,9 @@ static inline void dst_hold(struct dst_entry * dst)
 	 * If your kernel compilation stops here, please check
 	 * __pad_to_align_refcnt declaration in struct dst_entry
 	 */
+#if !defined(DEBUG_RCU_HEAD)
 	BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63);
+#endif
 	atomic_inc(&dst->__refcnt);
 }
 

Repetitive use of vmap/vunmap on the same address triggers this bug.

Simplest fix: directly kfree the data structure rather than doing it lazily.

Impact:
(-) slight slowdown on vunmap
(+) stop messing up rcu callback lists ;)

Caught it with DEBUG_RCU_HEAD, running LTTng armall/disarmall in loops. Immediate values (with breakpoint-ipi scheme) are still using vunmap.

------------[ cut here ]------------
WARNING: at kernel/rcutree.c:1199 __call_rcu+0x181/0x190()
Hardware name: X7DAL
Modules linked in: loop ltt_statedump ltt_userspace_event ipc_tr]
Pid: 4527, comm: ltt-armall Not tainted 2.6.30.9-trace #30
Call Trace:
 [<ffffffff8027b181>] ? __call_rcu+0x181/0x190
 [<ffffffff8027b181>] ? __call_rcu+0x181/0x190
 [<ffffffff8023a6a9>] ? warn_slowpath_common+0x79/0xd0
 [<ffffffff802b4890>] ? rcu_free_va+0x0/0x10
 [<ffffffff8027b181>] ? __call_rcu+0x181/0x190
 [<ffffffff802b5298>] ? __purge_vmap_area_lazy+0x1a8/0x1e0
 [<ffffffff802b59d4>] ? free_unmap_vmap_area_noflush+0x74/0x80
 [<ffffffff802b5a0e>] ? remove_vm_area+0x2e/0x80
 [<ffffffff802b5b35>] ? __vunmap+0x45/0xf0
 [<ffffffffa002e826>] ? ltt_statedump_start+0x7b6/0x820 [ltt_sta]
 [<ffffffff8068978a>] ? arch_imv_update+0x16a/0x2f0
 [<ffffffff8027dd13>] ? imv_update_range+0x53/0xa0
 [<ffffffff8026235b>] ? _module_imv_update+0x4b/0x60
 [<ffffffff80262385>] ? module_imv_update+0x15/0x30
 [<ffffffff80280049>] ? marker_probe_register+0x149/0xb90
 [<ffffffff8040c8e0>] ? ltt_vtrace+0x0/0x8c0
 [<ffffffff80409330>] ? ltt_marker_connect+0xd0/0x150
 [<ffffffff8040f8dc>] ? marker_enable_write+0xec/0x120
 [<ffffffff802c6cb8>] ? __dentry_open+0x268/0x350
 [<ffffffff80280b2c>] ? marker_probe_cb+0x9c/0x170
 [<ffffffff80280b2c>] ? marker_probe_cb+0x9c/0x170
 [<ffffffff802c973b>] ? vfs_write+0xcb/0x170
 [<ffffffff802c9984>] ? sys_write+0x64/0x130
 [<ffffffff8020beeb>] ? system_call_fastpath+0x16/0x1b
---[ end trace ef92443e716fa199 ]---
------------[ cut here ]------------

Signed-off-by Mathieu Desnoyers <[email protected]>

CC [email protected]

CC [email protected]

CC "Paul E. McKenney" <[email protected]>

CC [email protected]

CC [email protected]

---
 mm/vmalloc.c |   18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

Index: linux-2.6-lttng/mm/vmalloc.c
===================================================================

mm/vmalloc.c +2 -16

@@ -417,13 +417,6 @@ overflow:
 	return va;
 }
 
-static void rcu_free_va(struct rcu_head *head)
-{
-	struct vmap_area *va = container_of(head, struct vmap_area, rcu_head);
-
-	kfree(va);
-}
-
 static void __free_vmap_area(struct vmap_area *va)
 {
 	BUG_ON(RB_EMPTY_NODE(&va->rb_node));

@@ -431,7 +424,7 @@ static void __free_vmap_area(struct vmap
 	RB_CLEAR_NODE(&va->rb_node);
 	list_del_rcu(&va->list);
 
-	call_rcu(&va->rcu_head, rcu_free_va);
+	kfree(va);
 }
 
 /*

@@ -757,13 +750,6 @@ static struct vmap_block *new_vmap_block
 	return vb;
 }
 
-static void rcu_free_vb(struct rcu_head *head)
-{
-	struct vmap_block *vb = container_of(head, struct vmap_block, rcu_head);
-
-	kfree(vb);
-}
-
 static void free_vmap_block(struct vmap_block *vb)
 {
 	struct vmap_block *tmp;

@@ -778,7 +764,7 @@ static void free_vmap_block(struct vmap_
 	BUG_ON(tmp != vb);
 
 	free_unmap_vmap_area_noflush(vb->va);
-	call_rcu(&vb->rcu_head, rcu_free_vb);
+	kfree(vb);
 }
 
 static void *vb_alloc(unsigned long size, gfp_t gfp_mask)

Signature · 3 lines

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Simplest fix: directly kfree the data structure rather than doing it lazily.

Quoted 1 line · Mathieu Desnoyers

Simplest fix: directly kfree the data structure rather than doing it lazily.

The delay is necessary as far as I can tell for performance reasons. But Nick was the last one fiddling around with the subsystem as far as I remember. CCing him. May be he has a minute to think about a fix that preserved performance.

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Fix for vunmap, which happens to be one of them, is included.

Note that some false-positives might come up when call_rcu() is called on uninitialized struct rcu_head. The solution is, surprisingly enough, to initialize them.

Quoted 5 lines · Mathieu Desnoyers

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Fix for vunmap, which happens to be one of them, is included.

Quoted 3 lines

Note that some false-positives might come up when call_rcu() is called on uninitialized struct rcu_head. The solution is, surprisingly enough, to initialize them.

Very cool, Mathieu!!!

I do like adding the debug-only field to struct rcu_head. Lots of additional places need initialization, or am I missing a trick here?

Thanx, Paul

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Quoted 2 lines · Mathieu Desnoyers

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Nice idea - but any reason why this isnt using the debugojects framework? That debugging framework is upstream already and allows this kind of object lifetime debugging - in a much broader way.

Ingo

Quoted 10 lines · * Ingo Molnar ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Nice idea - but any reason why this isnt using the debugojects framework? That debugging framework is upstream already and allows this kind of object lifetime debugging - in a much broader way.

Other than: it did not occur to me, no, there is no reason for not using debugobjects there. :-)

As long as we can tag the object as
- initialized
- active
- inactive

And dump a nice fat warning when the object is activated twice, that should suffice. Now time is a bit short on my side, but I'll keep in mind to respin a version on top of debugobjects soon.

Quoted 15 lines · Mathieu Desnoyers

Quoted 10 lines · * Ingo Molnar ([email protected]) wrote:

Quoted 2 lines · Mathieu Desnoyers

Here is a patchset, done on 2.6.30.9, which permits to detect and fix racy call_rcu() users.

Nice idea - but any reason why this isnt using the debugojects framework? That debugging framework is upstream already and allows this kind of object lifetime debugging - in a much broader way.

Other than: it did not occur to me, no, there is no reason for not using debugobjects there. :-)

Quoted 8 lines

As long as we can tag the object as
- initialized
- active
- inactive

And dump a nice fat warning when the object is activated twice, that should suffice. Now time is a bit short on my side, but I'll keep in mind to respin a version on top of debugobjects soon.