Re: [PATCH v4 2/2] rcu: Dump memory object info if callback function is invalid

[Paul E. McKenney]

On Wed, Aug 02, 2023 at 09:09:18PM +0800, thunder.leizhen@xxxxxxxxxxxxxxx wrote:
> From: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> 
> When a structure containing an RCU callback rhp is (incorrectly) freed
> and reallocated after rhp is passed to call_rcu(), it is not unusual for
> rhp->func to be set to NULL. This defeats the debugging prints used by
> __call_rcu_common() in kernels built with CONFIG_DEBUG_OBJECTS_RCU_HEAD=y,
> which expect to identify the offending code using the identity of this
> function.
> 
> And in kernels build without CONFIG_DEBUG_OBJECTS_RCU_HEAD=y, things
> are even worse, as can be seen from this splat:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0
> ... ...
> PC is at 0x0
> LR is at rcu_do_batch+0x1c0/0x3b8
> ... ...
>  (rcu_do_batch) from (rcu_core+0x1d4/0x284)
>  (rcu_core) from (__do_softirq+0x24c/0x344)
>  (__do_softirq) from (__irq_exit_rcu+0x64/0x108)
>  (__irq_exit_rcu) from (irq_exit+0x8/0x10)
>  (irq_exit) from (__handle_domain_irq+0x74/0x9c)
>  (__handle_domain_irq) from (gic_handle_irq+0x8c/0x98)
>  (gic_handle_irq) from (__irq_svc+0x5c/0x94)
>  (__irq_svc) from (arch_cpu_idle+0x20/0x3c)
>  (arch_cpu_idle) from (default_idle_call+0x4c/0x78)
>  (default_idle_call) from (do_idle+0xf8/0x150)
>  (do_idle) from (cpu_startup_entry+0x18/0x20)
>  (cpu_startup_entry) from (0xc01530)
> 
> This commit therefore adds calls to mem_dump_obj(rhp) to output some
> information, for example:
> 
>   slab kmalloc-256 start ffff410c45019900 pointer offset 0 size 256
> 
> This provides the rough size of the memory block and the offset of the
> rcu_head structure, which as least provides at least a few clues to help
> locate the problem. If the problem is reproducible, additional slab
> debugging can be enabled, for example, CONFIG_DEBUG_SLAB=y, which can
> provide significantly more information.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>

Looks plausible, thank you!

What did you do to test this? 

One option is the object_debug module parameter to rcutorture, which is
described here: https://paulmck.livejournal.com/61432.html

> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>

Not a big problem, but not a good habit to get into...  I add my own
Signed-off-by when I pull patches into my tree.  Or if you are thinking
in terms of sending this to mainline using some other path, when I am
good with it, I would give you a tag to use.

So were you looking for me to take these two patches?

							Thanx, Paul

> ---
>  kernel/rcu/rcu.h      | 7 +++++++
>  kernel/rcu/srcutiny.c | 1 +
>  kernel/rcu/srcutree.c | 1 +
>  kernel/rcu/tasks.h    | 1 +
>  kernel/rcu/tiny.c     | 1 +
>  kernel/rcu/tree.c     | 1 +
>  6 files changed, 12 insertions(+)
> 
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index d1dcb09750efbd6..bc81582238b9846 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -10,6 +10,7 @@
>  #ifndef __LINUX_RCU_H
>  #define __LINUX_RCU_H
>  
> +#include <linux/slab.h>
>  #include <trace/events/rcu.h>
>  
>  /*
> @@ -248,6 +249,12 @@ static inline void debug_rcu_head_unqueue(struct rcu_head *head)
>  }
>  #endif	/* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
>  
> +static inline void debug_rcu_head_callback(struct rcu_head *rhp)
> +{
> +	if (unlikely(!rhp->func))
> +		kmem_dump_obj(rhp);
> +}
> +
>  extern int rcu_cpu_stall_suppress_at_boot;
>  
>  static inline bool rcu_stall_is_suppressed_at_boot(void)
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index 336af24e0fe358a..c38e5933a5d6937 100644
> --- a/kernel/rcu/srcutiny.c
> +++ b/kernel/rcu/srcutiny.c
> @@ -138,6 +138,7 @@ void srcu_drive_gp(struct work_struct *wp)
>  	while (lh) {
>  		rhp = lh;
>  		lh = lh->next;
> +		debug_rcu_head_callback(rhp);
>  		local_bh_disable();
>  		rhp->func(rhp);
>  		local_bh_enable();
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index f1a905200fc2f79..833a8f848a90ae6 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -1710,6 +1710,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>  	rhp = rcu_cblist_dequeue(&ready_cbs);
>  	for (; rhp != NULL; rhp = rcu_cblist_dequeue(&ready_cbs)) {
>  		debug_rcu_head_unqueue(rhp);
> +		debug_rcu_head_callback(rhp);
>  		local_bh_disable();
>  		rhp->func(rhp);
>  		local_bh_enable();
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 7294be62727b12c..148ac6a464bfb12 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -538,6 +538,7 @@ static void rcu_tasks_invoke_cbs(struct rcu_tasks *rtp, struct rcu_tasks_percpu
>  	raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
>  	len = rcl.len;
>  	for (rhp = rcu_cblist_dequeue(&rcl); rhp; rhp = rcu_cblist_dequeue(&rcl)) {
> +		debug_rcu_head_callback(rhp);
>  		local_bh_disable();
>  		rhp->func(rhp);
>  		local_bh_enable();
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index 42f7589e51e09e7..fec804b7908032d 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -97,6 +97,7 @@ static inline bool rcu_reclaim_tiny(struct rcu_head *head)
>  
>  	trace_rcu_invoke_callback("", head);
>  	f = head->func;
> +	debug_rcu_head_callback(head);
>  	WRITE_ONCE(head->func, (rcu_callback_t)0L);
>  	f(head);
>  	rcu_lock_release(&rcu_callback_map);
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 7c79480bfaa04e4..927c5ba0ae42269 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2135,6 +2135,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  		trace_rcu_invoke_callback(rcu_state.name, rhp);
>  
>  		f = rhp->func;
> +		debug_rcu_head_callback(rhp);
>  		WRITE_ONCE(rhp->func, (rcu_callback_t)0L);
>  		f(rhp);
>  
> -- 
> 2.34.1
>