RCU: introduce noref debug

We currently lack a debugging infrastructure to ensure that long-lived noref dst are properly handled - namely dropped or converted to a refcounted version before escaping the current RCU section.

This changeset implements such infra, tracking the noref pointer on a per CPU store and checking that all noref tracked at any given RCU nesting level are cleared before leaving such RCU section.

Each noref entity is identified using the entity pointer and an additional, optional, key/opaque pointer. This is needed to cope with usage case scenario where the same noref entity is stored in different places (e.g. noref dst on skb_clone()).

To keep the patch/implementation simple RCU_NOREF_DEBUG depends on PREEMPT_RCU=n in Kconfig.

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/rcupdate.h | 11 ++++++
 kernel/rcu/Kconfig.debug | 15 ++++++++
 kernel/rcu/Makefile      |  1 +
 kernel/rcu/noref_debug.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+)
 create mode 100644 kernel/rcu/noref_debug.c

include/linux/rcupdate.h +11 -0

index de50d8a4cf41..20c1ce08e3eb 100644
@@ -60,6 +60,15 @@ void call_rcu_sched(struct rcu_head *head, rcu_callback_t func);
 void synchronize_sched(void);
 void rcu_barrier_tasks(void);
 
+#ifdef CONFIG_RCU_NOREF_DEBUG
+void rcu_track_noref(void *key, void *noref, bool track);
+void __rcu_check_noref(void);
+
+#else
+static inline void rcu_track_noref(void *key, void *noref, bool add) { }
+static inline void __rcu_check_noref(void) { }
+#endif
+
 #ifdef CONFIG_PREEMPT_RCU
 
 void __rcu_read_lock(void);

@@ -85,6 +94,7 @@ static inline void __rcu_read_lock(void)
 
 static inline void __rcu_read_unlock(void)
 {
+	__rcu_check_noref();
 	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
 		preempt_enable();
 }

@@ -723,6 +733,7 @@ static inline void rcu_read_unlock_bh(void)
 			 "rcu_read_unlock_bh() used illegally while idle");
 	rcu_lock_release(&rcu_bh_lock_map);
 	__release(RCU_BH);
+	__rcu_check_noref();
 	local_bh_enable();
 }
 

kernel/rcu/Kconfig.debug +15 -0

index 0ec7d1d33a14..6c7f52a3e809 100644
@@ -68,6 +68,21 @@ config RCU_TRACE
 	  Say Y here if you want to enable RCU tracing
 	  Say N if you are unsure.
 
+config RCU_NOREF_DEBUG
+	bool "Debugging for RCU-protected noref entries"
+	depends on PREEMPT_RCU=n
+	select PREEMPT_COUNT
+	default n
+	help
+	  In case of long lasting rcu_read_lock sections this debug
+	  feature enables one to ensure that no rcu managed dereferenced
+	  data leaves the locked section. One use case is the tracking
+	  of dst_entries in struct sk_buff ->dst, which is used to pass
+	  the dst_entry around in the whole stack while under RCU lock.
+
+	  Say Y here if you want to enable noref RCU debugging
+	  Say N if you are unsure.
+
 config RCU_EQS_DEBUG
 	bool "Provide debugging asserts for adding NO_HZ support to an arch"
 	depends on DEBUG_KERNEL

kernel/rcu/Makefile +1 -0

index 13c0fc852767..c67d7c65c582 100644
@@ -11,3 +11,4 @@ obj-$(CONFIG_TREE_RCU) += tree.o
 obj-$(CONFIG_PREEMPT_RCU) += tree.o
 obj-$(CONFIG_TINY_RCU) += tiny.o
 obj-$(CONFIG_RCU_NEED_SEGCBLIST) += rcu_segcblist.o
+obj-$(CONFIG_RCU_NOREF_DEBUG) += noref_debug.o
\ No newline at end of file

kernel/rcu/noref_debug.c new file +20 -0

new file mode 100644
index 000000000000..ae2e104b11d6
@@ -0,0 +1,89 @@
+#include <linux/bug.h>
+#include <linux/percpu.h>
+#include <linux/skbuff.h>
+#include <linux/bitops.h>
+
+#define NOREF_MAX 7
+struct noref_entry {
+	void *noref;
+	void *key;
+	int nesting;
+};
+
+struct noref_cache {
+	struct noref_entry store[NOREF_MAX];
+};
+
+static DEFINE_PER_CPU(struct noref_cache, per_cpu_noref_cache);
+
+static int noref_cache_lookup(struct noref_cache *cache, void *noref, void *key)
+{

kernel/rcu/noref_debug.c new file +59 -0

+void rcu_track_noref(void *key, void *noref, bool track)
+{
+	struct noref_cache *cache = this_cpu_ptr(&per_cpu_noref_cache);
+	int i;
+
+	if (track) {
+		/* find the first empty slot */
+		i = noref_cache_lookup(cache, NULL, 0);
+		if (i < 0) {
+			WARN_ONCE(1, "can't find empty an slot to track a noref"
+				  " noref tracking will be inaccurate");
+			return;
+		}
+
+		cache->store[i].noref = noref;
+		cache->store[i].key = key;
+		cache->store[i].nesting = preempt_count();
+		return;
+	}
+
+	i = noref_cache_lookup(cache, noref, key);
+	if (i == -1) {
+		WARN_ONCE(1, "to-be-untracked noref entity %p not found in "
+			  "cache\n", noref);
+		return;
+	}
+
+	cache->store[i].noref = NULL;
+	cache->store[i].key = NULL;
+	cache->store[i].nesting = 0;
+}
+EXPORT_SYMBOL_GPL(rcu_track_noref);
+
+void __rcu_check_noref(void)
+{
+	struct noref_cache *cache = this_cpu_ptr(&per_cpu_noref_cache);
+	char *cur, buf[strlen("0xffffffffffffffff") * NOREF_MAX + 1];
+	int nesting = preempt_count();
+	int i, ret, cnt = 0;
+
+	cur = buf;
+	for (i = 0; i < NOREF_MAX; ++i) {
+		if (!cache->store[i].noref ||
+		    cache->store[i].nesting != nesting)
+			continue;
+
+		cnt++;
+		ret = sprintf(cur, " %p", cache->store[i].noref);
+		if (ret >= 0)
+			cur += ret;
+		cache->store[i].noref = NULL;
+		cache->store[i].key = NULL;
+		cache->store[i].nesting = 0;
+	}
+
+	WARN_ONCE(cnt, "%d noref entities escaped an RCU section, "
+		  "nesting %d, leaked noref list %s", cnt, nesting, buf);
+}
+EXPORT_SYMBOL_GPL(__rcu_check_noref);

Enabling CONFIG_RCU_NOREF_DEBUG gives the following splat when processing tcp packets:

           to-be-untracked noref entity ffff942cb71ea300 not found in cache
           ------------[ cut here ]------------
           WARNING: CPU: 24 PID: 178 at kernel/rcu/noref_debug.c:54 rcu_track_noref+0xa4/0xf0
           Modules linked in: intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd glue_helper cryptd iTCO_wdt ipmi_ssif mei_me iTCO_vendor_support mei dcdbas lpc_ich ipmi_si mxm_wmi sg pcspkr ipmi_devintf ipmi_msghandler acpi_power_meter shpchp wmi nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs libcrc32c sd_mod mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm igb drm ixgbe mdio crc32c_intel ahci ptp i2c_algo_bit libahci pps_core i2c_core libata dca dm_mirror dm_region_hash dm_log dm_mod
           CPU: 24 PID: 178 Comm: ksoftirqd/24 Not tainted 4.14.0-rc1.noref_route+ #1610
           Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.4.3 01/17/2017
           task: ffff940e48300000 task.stack: ffffaec406a20000
           RIP: 0010:rcu_track_noref+0xa4/0xf0
           RSP: 0018:ffffaec406a238e0 EFLAGS: 00010246
           RAX: 0000000000000040 RBX: 0000000000000000 RCX: 0000000000000002
           RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000292
           RBP: ffffaec406a238e0 R08: 0000000000000000 R09: 0000000000000000
           R10: 0000000000000001 R11: 0000000000000003 R12: 0000000000000000

           R13: ffff942cb5110000 R14: 000000000000fe88 R15: ffff942cb1a20200
           FS:  0000000000000000(0000) GS:ffff942cbee00000(0000) knlGS:0000000000000000
           CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
           CR2: 00007febc072d140 CR3: 0000001feebd6002 CR4: 00000000003606e0
           DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
           DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
           Call Trace:
            tcp_data_queue+0x82a/0xce0

            tcp_rcv_established+0x283/0x570
            tcp_v4_do_rcv+0x102/0x1e0
            tcp_v4_rcv+0xa9e/0xc10
            ip_local_deliver_finish+0x128/0x380
            ? ip_local_deliver_finish+0x41/0x380
            ip_local_deliver+0x74/0x230
            ip_rcv_finish+0x105/0x5e0
            ip_rcv+0x2a7/0x540
            __netif_receive_skb_core+0x3b9/0xe10
            ? netif_receive_skb_internal+0x40/0x390
            __netif_receive_skb+0x18/0x60
            netif_receive_skb_internal+0x8d/0x390
            ? netif_receive_skb_internal+0x40/0x390
            napi_gro_complete+0x127/0x1d0
            ? napi_gro_complete+0x2a/0x1d0
            napi_gro_flush+0x5f/0x80
            napi_complete_done+0x96/0x100
            ixgbe_poll+0x5f8/0x7c0 [ixgbe]
            net_rx_action+0x27d/0x520
            __do_softirq+0xd1/0x4f5
            run_ksoftirqd+0x25/0x70
            smpboot_thread_fn+0x11a/0x1f0
            kthread+0x155/0x190
            ? sort_range+0x30/0x30
            ? kthread_create_on_node+0x70/0x70
            ret_from_fork+0x2a/0x40
           Code: e9 83 c2 01 48 83 c0 18 83 fa 07 75 ef 80 3d b0 e5 ff 00 00 75 d2 48 c7 c7 50 54 07 9d 31 c0 c6 05 9e e5 ff 00 01 e8 ef af fe ff <0f> ff 5d c3 80 3d 8f e5 ff 00 00 75 b0 48 c7 c7 00 54 07 9d 31

we must clear the skb dst before enqueuing the skb somewhere, but currently the dst entry for packets delivered via tcp_rcv_established() -> tcp_queue_rcv() is not cleared.

Fix it by adding the explicit drop in tcp_queue_rcv() and moving the current skb_dst_drop() just before the other enqueuing operation, do avoid unneeded double skb_dst_drop() for some path.

The leak itself is not harmful, because the tcp recvmsg() code should not access such info.

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv4/tcp_input.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/ipv4/tcp_input.c +4 -1

index c5d7656beeee..bf4e17edfe7a 100644
@@ -4422,6 +4422,9 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
 		return;
 	}
 
+	/* drop the -possibly noref - dst before delivery the skb to ofo tree */
+	skb_dst_drop(skb);
+
 	/* Stash tstamp to avoid being stomped on by rbnode */
 	if (TCP_SKB_CB(skb)->has_rxtstamp)
 		TCP_SKB_CB(skb)->swtstamp = skb->tstamp;

@@ -4560,6 +4563,7 @@ static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int
 				  skb, fragstolen)) ? 1 : 0;
 	tcp_rcv_nxt_update(tcp_sk(sk), TCP_SKB_CB(skb)->end_seq);
 	if (!eaten) {
+		skb_dst_drop(skb);
 		__skb_queue_tail(&sk->sk_receive_queue, skb);
 		skb_set_owner_r(skb, sk);
 	}

@@ -4626,7 +4630,6 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
 		__kfree_skb(skb);
 		return;
 	}
-	skb_dst_drop(skb);
 	__skb_pull(skb, tcp_hdr(skb)->doff * 4);
 
 	tcp_ecn_accept_cwr(tp, skb);

Quoted 16 lines · Paolo Abeni

Enabling CONFIG_RCU_NOREF_DEBUG gives the following splat when processing tcp packets:

           to-be-untracked noref entity ffff942cb71ea300 not found in cache
           ------------[ cut here ]------------
           WARNING: CPU: 24 PID: 178 at kernel/rcu/noref_debug.c:54 rcu_track_noref+0xa4/0xf0
           Modules linked in: intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd glue_helper cryptd iTCO_wdt ipmi_ssif mei_me iTCO_vendor_support mei dcdbas lpc_ich ipmi_si mxm_wmi sg pcspkr ipmi_devintf ipmi_msghandler acpi_power_meter shpchp wmi nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs libcrc32c sd_mod mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm igb drm ixgbe mdio crc32c_intel ahci ptp i2c_algo_bit libahci pps_core i2c_core libata dca dm_mirror dm_region_hash dm_log dm_mod
           CPU: 24 PID: 178 Comm: ksoftirqd/24 Not tainted 4.14.0-rc1.noref_route+ #1610
           Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.4.3 01/17/2017
           task: ffff940e48300000 task.stack: ffffaec406a20000
           RIP: 0010:rcu_track_noref+0xa4/0xf0
           RSP: 0018:ffffaec406a238e0 EFLAGS: 00010246
           RAX: 0000000000000040 RBX: 0000000000000000 RCX: 0000000000000002
           RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000292
           RBP: ffffaec406a238e0 R08: 0000000000000000 R09: 0000000000000000
           R10: 0000000000000001 R11: 0000000000000003 R12: 0000000000000000

Quoted 8 lines

           R13: ffff942cb5110000 R14: 000000000000fe88 R15: ffff942cb1a20200
           FS:  0000000000000000(0000) GS:ffff942cbee00000(0000) knlGS:0000000000000000
           CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
           CR2: 00007febc072d140 CR3: 0000001feebd6002 CR4: 00000000003606e0
           DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
           DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
           Call Trace:
            tcp_data_queue+0x82a/0xce0

That is strange, since tcp_data_queue() starts with

	skb_dst_drop(skb); 

So this stack trace looks suspicious.

Hi,

Quoted 4 lines · Eric Dumazet

Quoted 3 lines · Paolo Abeni

Enabling CONFIG_RCU_NOREF_DEBUG gives the following splat when processing tcp packets:

Quoted 12 lines

Quoted 4 lines

           to-be-untracked noref entity ffff942cb71ea300 not found in cache
           ------------[ cut here ]------------
           WARNING: CPU: 24 PID: 178 at kernel/rcu/noref_debug.c:54 rcu_track_noref+0xa4/0xf0
           Modules linked in: intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd glue_helper cryptd iTCO_wdt ipmi_ssif mei_me iTCO_vendor_support mei dcdbas lpc_ich ipmi_si mxm_wmi sg pcspkr ipmi_devintf ipmi_msghandler acpi_power_meter shpchp wmi nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs libcrc32c sd_mod mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm igb drm ixgbe mdio crc32c_intel ahci ptp i2c_algo_bit libahci pps_core i2c_core libata dca dm_mirror dm_region_hash dm_log dm_mod

Quoted 8 lines

           CPU: 24 PID: 178 Comm: ksoftirqd/24 Not tainted 4.14.0-rc1.noref_route+ #1610
           Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.4.3 01/17/2017
           task: ffff940e48300000 task.stack: ffffaec406a20000
           RIP: 0010:rcu_track_noref+0xa4/0xf0
           RSP: 0018:ffffaec406a238e0 EFLAGS: 00010246
           RAX: 0000000000000040 RBX: 0000000000000000 RCX: 0000000000000002
           RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000292
           RBP: ffffaec406a238e0 R08: 0000000000000000 R09: 0000000000000000

Thank you for the feedback.

I most probably messed-up while extracting the info from dmsg, as this issue gives a couple of splats almost concurrently. Please let me re-do the test and post a more resonable dmsg.

The problem with the current code is that in the tcp_rcv_established() -> tcp_queue_rcv() path, the skb_dst() is not cleared.

The networking subsystem is currently using some kind of long-lived RCU-protected, references to avoid the overhead of full book-keeping.

Such references - skb_dst() noref - are stored inside the skbs and can be moved across relevant slices of the network stack, with the users being in charge of properly clearing the relevant skb - or properly refcount the related dst references - before the skb escapes the RCU section.

We currently don't have any deterministic debug infrastructure to check the dst noref usages - and the introduction of others noref artifact is currently under discussion.

This series tries to tackle the above introducing an RCU debug infrastructure aimed at spotting incorrect noref pointer usage, in patch one. The infrastructure is small and must be explicitly enabled via a newly introduced build option.

Patch two uses such infrastructure to track dst noref usage in the networking stack.

Patch 3 and 4 are bugfixes for small buglet found running this infrastructure on basic scenarios.

Paolo Abeni (4):
rcu: introduce noref debug
net: use RCU noref infrastructure to track dst noref
ipv4: drop unneeded and misleading RCU lock in ip_route_input_noref()
tcp: avoid noref dst leak on input path

 include/linux/rcupdate.h | 11 ++++++
 include/linux/skbuff.h   |  1 +
 include/net/dst.h        |  5 +++
 kernel/rcu/Kconfig.debug | 15 ++++++++
 kernel/rcu/Makefile      |  1 +
 kernel/rcu/noref_debug.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/route.c         |  7 +---
 net/ipv4/tcp_input.c     |  5 ++-
 8 files changed, 127 insertions(+), 7 deletions(-)
 create mode 100644 kernel/rcu/noref_debug.c

Signature · 1 line

2.13.6

Quoted 17 lines · Paolo Abeni

The networking subsystem is currently using some kind of long-lived RCU-protected, references to avoid the overhead of full book-keeping.

Such references - skb_dst() noref - are stored inside the skbs and can be moved across relevant slices of the network stack, with the users being in charge of properly clearing the relevant skb - or properly refcount the related dst references - before the skb escapes the RCU section.

We currently don't have any deterministic debug infrastructure to check the dst noref usages - and the introduction of others noref artifact is currently under discussion.

This series tries to tackle the above introducing an RCU debug infrastructure aimed at spotting incorrect noref pointer usage, in patch one. The infrastructure is small and must be explicitly enabled via a newly introduced build option.

Quoted 5 lines

Patch two uses such infrastructure to track dst noref usage in the networking stack.

Patch 3 and 4 are bugfixes for small buglet found running this infrastructure on basic scenarios.

This patchset does not look like it handles rcu_read_lock() nesting. For example, given code like this:

	void foo(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		do_something();
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void bar(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo();
		do_something_else();
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grill(void)
	{
		foo();
	}

It looks like foo()'s rcu_read_unlock() will complain about key1. You could remove foo()'s rcu_read_lock() and rcu_read_unlock(), but that will break the call from grill().

Or am I missing something subtle here? Given patch 3/4, I suspect not...

Hi,

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Paolo Abeni

The networking subsystem is currently using some kind of long-lived RCU-protected, references to avoid the overhead of full book-keeping.

Such references - skb_dst() noref - are stored inside the skbs and can be moved across relevant slices of the network stack, with the users being in charge of properly clearing the relevant skb - or properly refcount the related dst references - before the skb escapes the RCU section.

We currently don't have any deterministic debug infrastructure to check the dst noref usages - and the introduction of others noref artifact is currently under discussion.

This series tries to tackle the above introducing an RCU debug infrastructure aimed at spotting incorrect noref pointer usage, in patch one. The infrastructure is small and must be explicitly enabled via a newly introduced build option.

Quoted 5 lines

Quoted 5 lines

Patch two uses such infrastructure to track dst noref usage in the networking stack.

Patch 3 and 4 are bugfixes for small buglet found running this infrastructure on basic scenarios.

Thank you for the prompt reply!

Quoted 29 lines

This patchset does not look like it handles rcu_read_lock() nesting. For example, given code like this:

	void foo(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		do_something();
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void bar(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo();
		do_something_else();
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grill(void)
	{
		foo();
	}

Quoted 3 lines

It looks like foo()'s rcu_read_unlock() will complain about key1. You could remove foo()'s rcu_read_lock() and rcu_read_unlock(), but that will break the call from grill().

Actually the code should cope correctly with your example; when foo()'s rcu_read_unlock() is called, 'cache' contains:

{ { &key1, &noref1, 1}, // ...

and when the related __rcu_check_noref() is invoked preempt_count() is 2 - because the check is called before decreasing the preempt counter.

In the main loop inside __rcu_check_noref() we will hit always the 'continue' statement because 'cache->store[i].nesting != nesting', so no warn will be triggered.

Quoted 1 line

Or am I missing something subtle here? Given patch 3/4, I suspect not...

The problem with the code in patch 3/4 is different; currently ip_route_input_noref() is basically doing:

rcu_read_lock();

rcu_track_noref(&key1, &noref1, true);

rcu_read_unlock();

So the rcu lock there silence any RCU based check inside ip_route_input_noref() but does not really protect the noref dst.

Please let me know if the above clarify the scenario.

Quoted 64 lines · Paolo Abeni

Hi,

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Paolo Abeni

The networking subsystem is currently using some kind of long-lived RCU-protected, references to avoid the overhead of full book-keeping.

Such references - skb_dst() noref - are stored inside the skbs and can be moved across relevant slices of the network stack, with the users being in charge of properly clearing the relevant skb - or properly refcount the related dst references - before the skb escapes the RCU section.

We currently don't have any deterministic debug infrastructure to check the dst noref usages - and the introduction of others noref artifact is currently under discussion.

This series tries to tackle the above introducing an RCU debug infrastructure aimed at spotting incorrect noref pointer usage, in patch one. The infrastructure is small and must be explicitly enabled via a newly introduced build option.

Quoted 5 lines

Quoted 5 lines

Patch two uses such infrastructure to track dst noref usage in the networking stack.

Patch 3 and 4 are bugfixes for small buglet found running this infrastructure on basic scenarios.

Thank you for the prompt reply!

Quoted 29 lines

This patchset does not look like it handles rcu_read_lock() nesting. For example, given code like this:

	void foo(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		do_something();
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void bar(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo();
		do_something_else();
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grill(void)
	{
		foo();
	}

Quoted 3 lines

It looks like foo()'s rcu_read_unlock() will complain about key1. You could remove foo()'s rcu_read_lock() and rcu_read_unlock(), but that will break the call from grill().

Actually the code should cope correctly with your example; when foo()'s rcu_read_unlock() is called, 'cache' contains:

Quoted 8 lines

{ { &key1, &noref1, 1}, // ...

and when the related __rcu_check_noref() is invoked preempt_count() is 2 - because the check is called before decreasing the preempt counter.

In the main loop inside __rcu_check_noref() we will hit always the 'continue' statement because 'cache->store[i].nesting != nesting', so no warn will be triggered.

You are right, it was too early, and my example wasn't correct. How about this one?

	void foo(void (*f)(struct s *sp), struct s **spp)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		f(spp);
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void barcb(struct s **spp)
	{
		*spp = &noref3;
		rcu_track_noref(&key3, *spp, true);
	}

	void bar(void)
	{
		struct s *sp;

		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo(barcb, &sp);
		do_something_else(sp);
		rcu_track_noref(&key3, sp, false);
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grillcb(struct s **spp)

	{
		*spp
	}

	void grill(void)
	{
		foo();
	}

How does the user select the key argument? It looks like someone can choose to just pass in NULL -- is that the intent, or am I confused about this as well?

I am also concerned about false negatives when the user invokes rcu_track_noref(..., false) but then leaks the pointer anyway. Or is there something you are doing that catches this that I am missing?

Quoted 107 lines · Paul E. McKenney

Quoted 64 lines · Paolo Abeni

Hi,

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Paolo Abeni

The networking subsystem is currently using some kind of long-lived RCU-protected, references to avoid the overhead of full book-keeping.

Such references - skb_dst() noref - are stored inside the skbs and can be moved across relevant slices of the network stack, with the users being in charge of properly clearing the relevant skb - or properly refcount the related dst references - before the skb escapes the RCU section.

We currently don't have any deterministic debug infrastructure to check the dst noref usages - and the introduction of others noref artifact is currently under discussion.

This series tries to tackle the above introducing an RCU debug infrastructure aimed at spotting incorrect noref pointer usage, in patch one. The infrastructure is small and must be explicitly enabled via a newly introduced build option.

Quoted 5 lines

Quoted 5 lines

Patch two uses such infrastructure to track dst noref usage in the networking stack.

Patch 3 and 4 are bugfixes for small buglet found running this infrastructure on basic scenarios.

Thank you for the prompt reply!

Quoted 29 lines

This patchset does not look like it handles rcu_read_lock() nesting. For example, given code like this:

	void foo(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		do_something();
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void bar(void)
	{
		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo();
		do_something_else();
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grill(void)
	{
		foo();
	}

Quoted 3 lines

It looks like foo()'s rcu_read_unlock() will complain about key1. You could remove foo()'s rcu_read_lock() and rcu_read_unlock(), but that will break the call from grill().

Actually the code should cope correctly with your example; when foo()'s rcu_read_unlock() is called, 'cache' contains:

Quoted 8 lines

{ { &key1, &noref1, 1}, // ...

and when the related __rcu_check_noref() is invoked preempt_count() is 2 - because the check is called before decreasing the preempt counter.

In the main loop inside __rcu_check_noref() we will hit always the 'continue' statement because 'cache->store[i].nesting != nesting', so no warn will be triggered.

You are right, it was too early, and my example wasn't correct. How about this one?

	void foo(void (*f)(struct s *sp), struct s **spp)
	{
		rcu_read_lock();
		rcu_track_noref(&key2, &noref2, true);
		f(spp);
		rcu_track_noref(&key2, &noref2, false);
		rcu_read_unlock();
	}

	void barcb(struct s **spp)
	{
		*spp = &noref3;
		rcu_track_noref(&key3, *spp, true);
	}

	void bar(void)
	{
		struct s *sp;

		rcu_read_lock();
		rcu_track_noref(&key1, &noref1, true);
		do_something_more();
		foo(barcb, &sp);
		do_something_else(sp);
		rcu_track_noref(&key3, sp, false);
		rcu_track_noref(&key1, &noref1, false);
		rcu_read_unlock();
	}

	void grillcb(struct s **spp)

Quoted 8 lines

	{
		*spp
	}

	void grill(void)
	{
		foo();
	}

You are right: this will generate a splat, even if the code it safe. The false positive can be avoided looking for leaked references only in the outermost rcu unlook. I did a previous implementation performing such check, but it emitted very generic splat so I tried to be more strict. The latter choice allowed to find/do 3/4.

What about using save_stack_trace() in rcu_track_noref(, true) and reporting such stack trace when the check in the outer most rcu fails?

the current strict/false-postive-prone check could be enabled under an additional build flag.

Quoted 3 lines

How does the user select the key argument? It looks like someone can choose to just pass in NULL -- is that the intent, or am I confused about this as well?

The 'key' argument is intented to discriminate the scope of the same noref dst attached to different skbs, which happens e.g. as a result of as skb_dst_copy().

In a generic use case, it can be NULL, too.

Quoted 3 lines

I am also concerned about false negatives when the user invokes rcu_track_noref(..., false) but then leaks the pointer anyway. Or is there something you are doing that catches this that I am missing?

If the rcu_track_noref(..., false) is misplaced or missing, yes we can have false negative.

In the noref rcu use-case the rcu_track_noref(, false) call is/must be placed side-by-side with the code clearing the the noref pointer, so is/should be quite easy avoiding such mistakes.

Linus and Ingo will ask me how users decide how they should set that additional build flag. Especially given that if there is code that requires non-strict checking, isn't everyone required to set up non-strict checking to avoid false positives? Unless you can also configure out all the code that requires non-strict checking, I suppose, but how would you keep track of what to configure out?

OK. There will probably be some discussion about the API in that case.

True enough. Except that if people were good about always clearing the pointer, then the pointer couldn't leak, right? Or am I missing something in your use cases?

Or to put it another way -- have you been able to catch any real pointer-leak bugs with thister-leak bugs with this? The other RCU debug options have had pretty long found-bug lists before we accepted them.

Quoted 6 lines · Paul E. McKenney

Linus and Ingo will ask me how users decide how they should set that additional build flag. Especially given that if there is code that requires non-strict checking, isn't everyone required to set up non-strict checking to avoid false positives? Unless you can also configure out all the code that requires non-strict checking, I suppose, but how would you keep track of what to configure out?

I'm working to a new version using a single compile flag - without additional strict option.

I don't know of any other subsytem that stores rcu pointer in datastructures for a longer amount of time. That having said, I wonder if the tests should completely move to the networking subsystem for the time being. The Kconfig option would thus be called NET_DEBUG or something along the lines. For abstraction it would be possible to add an atomic_notifier_chain to the rcu_read/unlock methods, where multiple users or checkers could register for. That way we keep the users seperate from the implementation for the cost of a bit more layering and more indirect calls. But given that this will anyway slow down execution a lot, it will anyway only be suitable in testing/verification/debugging environments.

Quoted 1 line

OK. There will probably be some discussion about the API in that case.

I'll drop noref parameter, the key will became mandatory - the exact position of where the reference of RCU managed object is stored. In the case of noref dst it is &skb->_skb_refdst. With this kind of API it should suite more subsystems.

Quoted 3 lines

True enough. Except that if people were good about always clearing the pointer, then the pointer couldn't leak, right? Or am I missing something in your use cases?

This is correct. The dst_entry checking in skb, which this patch series implements there are strict brackets in the sense of skb_dst_set, skb_dst_set_noref, skb_dst_force, etc., which form brackets around the safe uses of those dst_entries. This patch series validates that the correct skb_dst_* functions are being called before the sk_buff leaves the rcu protected section. Thus we don't need to modify and review a lot of code but we can just patch into those helpers already.

Quoted 4 lines

Or to put it another way -- have you been able to catch any real pointer-leak bugs with thister-leak bugs with this? The other RCU debug options have had pretty long found-bug lists before we accepted them.

There have been two problems found so far, one is a rather minor one while the other one seems like a normal bug. The patches for those are part of this series (3/4 and 4/4).