Re: [PATCH 1/4] rcu: introduce noref debug

[Steven Rostedt]

On Fri,  6 Oct 2017 14:57:46 +0200
Paolo Abeni <pabeni@xxxxxxxxxx> wrote:

> 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 <pabeni@xxxxxxxxxx>
> ---
>  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
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index de50d8a4cf41..20c1ce08e3eb 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -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();
>  }
>  
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 0ec7d1d33a14..6c7f52a3e809 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -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
> diff --git a/kernel/rcu/Makefile b/kernel/rcu/Makefile
> index 13c0fc852767..c67d7c65c582 100644
> --- a/kernel/rcu/Makefile
> +++ b/kernel/rcu/Makefile
> @@ -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
> diff --git a/kernel/rcu/noref_debug.c b/kernel/rcu/noref_debug.c
> new file mode 100644
> index 000000000000..ae2e104b11d6
> --- /dev/null
> +++ b/kernel/rcu/noref_debug.c
> @@ -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)
> +{
> +	int i;
> +
> +	for (i = 0; i < NOREF_MAX; ++i)
> +		if (cache->store[i].noref == noref &&
> +		    cache->store[i].key == key)
> +			return i;
> +
> +	return -1;
> +}
> +

Please add a comment above this function on how to use it.

-- Steve

> +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);