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

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

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

v3 --> v4:
1. Remove kmem_valid_obj() and convert kmem_dump_obj() to work the same way
   as vmalloc_dump_obj().
2. In kernel/rcu/rcu.h
-#include <linux/mm.h>
+#include <linux/slab.h>

v2 --> v3:
1. I made statistics about the source of 'rhp'. kmem_valid_obj() accounts for
   more than 97.5%, and vmalloc accounts for less than 1%. So change call
   mem_dump_obj() to call kmem_dump_obj() can meet debugging requirements and
   avoid the potential deadlock risk of vmalloc_dump_obj().
-		mem_dump_obj(rhp);
+		if (kmem_valid_obj(rhp))
+			kmem_dump_obj(rhp);

   The discussion about vmap_area_lock deadlock in v2:
   https://lkml.org/lkml/2022/11/11/493

2. Provide static inline empty functions for kmem_valid_obj() and kmem_dump_obj()
   when CONFIG_PRINTK=n.

v1 --> v2:
1. Remove condition "(unsigned long)rhp->func & 0x3", it have problems on x86.
2. Paul E. McKenney helped me update the commit message, thanks.


Zhen Lei (2):
  mm: Remove kmem_valid_obj()
  rcu: Dump memory object info if callback function is invalid

 include/linux/slab.h  |  5 +++--
 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 +
 mm/slab_common.c      | 41 +++++++++++------------------------------
 mm/util.c             |  4 +---
 9 files changed, 27 insertions(+), 35 deletions(-)

-- 
2.34.1

[PATCH v4 1/2] mm: Remove kmem_valid_obj()

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

Function kmem_dump_obj() will splat if passed a pointer to a non-slab
object. So no one will call it directly. It is always necessary to call
kmem_valid_obj() first to determine whether the passed pointer to a
valid slab object. Then merging kmem_valid_obj() into kmem_dump_obj()
will make the code more concise. So convert kmem_dump_obj() to work the
same way as vmalloc_dump_obj(). After this, no one calls kmem_valid_obj()
anymore, and it can be safely removed.

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/slab.h |  5 +++--
 mm/slab_common.c     | 41 +++++++++++------------------------------
 mm/util.c            |  4 +---
 3 files changed, 15 insertions(+), 35 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 848c7c82ad5ad0b..d8ed2e810ec4448 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -244,8 +244,9 @@ DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
 size_t ksize(const void *objp);
 
 #ifdef CONFIG_PRINTK
-bool kmem_valid_obj(void *object);
-void kmem_dump_obj(void *object);
+bool kmem_dump_obj(void *object);
+#else
+static inline bool kmem_dump_obj(void *object) { return false; }
 #endif
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index d1555ea2981ac51..ee6ed6dd7ba9fa5 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -528,26 +528,6 @@ bool slab_is_available(void)
 }
 
 #ifdef CONFIG_PRINTK
-/**
- * kmem_valid_obj - does the pointer reference a valid slab object?
- * @object: pointer to query.
- *
- * Return: %true if the pointer is to a not-yet-freed object from
- * kmalloc() or kmem_cache_alloc(), either %true or %false if the pointer
- * is to an already-freed object, and %false otherwise.
- */
-bool kmem_valid_obj(void *object)
-{
-	struct folio *folio;
-
-	/* Some arches consider ZERO_SIZE_PTR to be a valid address. */
-	if (object < (void *)PAGE_SIZE || !virt_addr_valid(object))
-		return false;
-	folio = virt_to_folio(object);
-	return folio_test_slab(folio);
-}
-EXPORT_SYMBOL_GPL(kmem_valid_obj);
-
 static void kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab)
 {
 	if (__kfence_obj_info(kpp, object, slab))
@@ -566,11 +546,11 @@ static void kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *
  * and, if available, the slab name, return address, and stack trace from
  * the allocation and last free path of that object.
  *
- * This function will splat if passed a pointer to a non-slab object.
- * If you are not sure what type of object you have, you should instead
- * use mem_dump_obj().
+ * Return: %true if the pointer is to a not-yet-freed object from
+ * kmalloc() or kmem_cache_alloc(), either %true or %false if the pointer
+ * is to an already-freed object, and %false otherwise.
  */
-void kmem_dump_obj(void *object)
+bool kmem_dump_obj(void *object)
 {
 	char *cp = IS_ENABLED(CONFIG_MMU) ? "" : "/vmalloc";
 	int i;
@@ -578,13 +558,13 @@ void kmem_dump_obj(void *object)
 	unsigned long ptroffset;
 	struct kmem_obj_info kp = { };
 
-	if (WARN_ON_ONCE(!virt_addr_valid(object)))
-		return;
+	/* Some arches consider ZERO_SIZE_PTR to be a valid address. */
+	if (object < (void *)PAGE_SIZE || !virt_addr_valid(object))
+		return false;
 	slab = virt_to_slab(object);
-	if (WARN_ON_ONCE(!slab)) {
-		pr_cont(" non-slab memory.\n");
-		return;
-	}
+	if (!slab)
+		return false;
+
 	kmem_obj_info(&kp, object, slab);
 	if (kp.kp_slab_cache)
 		pr_cont(" slab%s %s", cp, kp.kp_slab_cache->name);
@@ -621,6 +601,7 @@ void kmem_dump_obj(void *object)
 		pr_info("    %pS\n", kp.kp_free_stack[i]);
 	}
 
+	return true;
 }
 EXPORT_SYMBOL_GPL(kmem_dump_obj);
 #endif
diff --git a/mm/util.c b/mm/util.c
index dd12b9531ac4cad..ddfbb22dc1876d3 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1063,10 +1063,8 @@ void mem_dump_obj(void *object)
 {
 	const char *type;
 
-	if (kmem_valid_obj(object)) {
-		kmem_dump_obj(object);
+	if (kmem_dump_obj(object))
 		return;
-	}
 
 	if (vmalloc_dump_obj(object))
 		return;
-- 
2.34.1

Re: [PATCH v4 1/2] mm: Remove kmem_valid_obj()

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 09:09:17PM +0800, thunder.leizhen@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Function kmem_dump_obj() will splat if passed a pointer to a non-slab
> object. So no one will call it directly. It is always necessary to call
> kmem_valid_obj() first to determine whether the passed pointer to a
> valid slab object. Then merging kmem_valid_obj() into kmem_dump_obj()
> will make the code more concise. So convert kmem_dump_obj() to work the
> same way as vmalloc_dump_obj(). After this, no one calls kmem_valid_obj()
> anymore, and it can be safely removed.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH v4 1/2] mm: Remove kmem_valid_obj()

[Vlastimil Babka]

On 8/2/23 15:09, thunder.leizhen@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Function kmem_dump_obj() will splat if passed a pointer to a non-slab
> object. So no one will call it directly. It is always necessary to call
> kmem_valid_obj() first to determine whether the passed pointer to a
> valid slab object. Then merging kmem_valid_obj() into kmem_dump_obj()
> will make the code more concise. So convert kmem_dump_obj() to work the
> same way as vmalloc_dump_obj(). After this, no one calls kmem_valid_obj()
> anymore, and it can be safely removed.
> 
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

No problem if this goes through rcu tree due to patch 2/2.

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

[thunder.leizhen]

From: Zhen Lei <thunder.leizhen@huawei.com>

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@huawei.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 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

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@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> 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@huawei.com>

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@kernel.org>

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
> 

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

[Leizhen (ThunderTown)]

On 2023/8/3 6:40, Paul E. McKenney wrote:
> On Wed, Aug 02, 2023 at 09:09:18PM +0800, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> 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@huawei.com>
> 
> Looks plausible, thank you!
> 
> What did you do to test this? 

This test is easier. I wrote a simple one myself.

static struct my_rcu_node *my_node;

static bool test_kmem_dump_obj(void)
{
        void *p;

        if (kmem_dump_obj(NULL))
                return false;

        if (kmem_dump_obj((void *)(PAGE_SIZE / 2)))
                return false;

        if (kmem_dump_obj((void *)(PAGE_SIZE - 1)))
                return false;

        if (kmem_dump_obj((void *)PAGE_SIZE))
                return false;

        if (kmem_dump_obj(&my_node))
                return false;

        p = vmalloc(0x100000);
        WARN_ON(!p);
        if (kmem_dump_obj(p)) {
                vfree(p);
                return false;
        }
        vfree(p);

        p = kmalloc(0x100, GFP_KERNEL);
        WARN_ON(!p);
        if (!kmem_dump_obj(p)) {
                kfree(p);
                return false;
        }
        if (kmem_dump_obj((void *)(((unsigned long)p << 4) >> 4))) {
                kfree(p);
                return false;
        }
        kfree(p);

        return true;
}

static int tst_proc_show(struct seq_file *m, void *v)
{
        if (!test_kmem_dump_obj()) {
                seq_printf(m, "test_kmem_dump_obj failed\n");
                return 0;
        }

        my_node = kmalloc(sizeof(*my_node), GFP_KERNEL);
        if (!my_node) {
                seq_printf(m, "kmalloc failed\n");
                return 0;
        }

        call_rcu(&my_node->node, my_rcu_cb);
        my_node->node.func = NULL;

        return 0;
}


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

OK, thanks for your info. I'll study the RCU self-test program rcutorture later.

> 
>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> 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.

Oh, Sorry. It seems that I forgot to delete your Signed-off-by in v2.
Oops! you reminded me once before. After v1, you helped modify the
description and pull it into your tree. I got it from 'dev' branch.

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

Yes, it could be quicker. Of course, I can wait for patch 1/2 upstream,
then repost patch 2/2. In fact, I also want to dump part of the slab
object, I've already written the code. In order not to affect the current
user of mem_dump_obj(), a new parameter need to be added to kmem_dump_obj().
I will post v5 with this patch later.

> 
> 							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
>>
> .
> 

-- 
Regards,
  Zhen Lei

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

[Leizhen (ThunderTown)]

On 2023/8/3 11:23, Leizhen (ThunderTown) wrote:
> 
> 
> On 2023/8/3 6:40, Paul E. McKenney wrote:
>> On Wed, Aug 02, 2023 at 09:09:18PM +0800, thunder.leizhen@huaweicloud.com wrote:
>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> 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@huawei.com>
>>
>> Looks plausible, thank you!
>>
>> What did you do to test this? 
> 
> This test is easier. I wrote a simple one myself.
> 
> static struct my_rcu_node *my_node;
> 
> static bool test_kmem_dump_obj(void)
> {
>         void *p;
> 
>         if (kmem_dump_obj(NULL))
>                 return false;
> 
>         if (kmem_dump_obj((void *)(PAGE_SIZE / 2)))
>                 return false;
> 
>         if (kmem_dump_obj((void *)(PAGE_SIZE - 1)))
>                 return false;
> 
>         if (kmem_dump_obj((void *)PAGE_SIZE))
>                 return false;
> 
>         if (kmem_dump_obj(&my_node))
>                 return false;
> 
>         p = vmalloc(0x100000);
>         WARN_ON(!p);
>         if (kmem_dump_obj(p)) {
>                 vfree(p);
>                 return false;
>         }
>         vfree(p);
> 
>         p = kmalloc(0x100, GFP_KERNEL);
>         WARN_ON(!p);
>         if (!kmem_dump_obj(p)) {
>                 kfree(p);
>                 return false;
>         }
>         if (kmem_dump_obj((void *)(((unsigned long)p << 4) >> 4))) {
>                 kfree(p);
>                 return false;
>         }
>         kfree(p);
> 
>         return true;
> }
> 
> static int tst_proc_show(struct seq_file *m, void *v)
> {
>         if (!test_kmem_dump_obj()) {
>                 seq_printf(m, "test_kmem_dump_obj failed\n");
>                 return 0;
>         }
> 
>         my_node = kmalloc(sizeof(*my_node), GFP_KERNEL);
>         if (!my_node) {
>                 seq_printf(m, "kmalloc failed\n");
>                 return 0;
>         }
> 
>         call_rcu(&my_node->node, my_rcu_cb);
>         my_node->node.func = NULL;
> 
>         return 0;
> }
> 
> 
>>
>> One option is the object_debug module parameter to rcutorture, which is
>> described here: https://paulmck.livejournal.com/61432.html
> 
> OK, thanks for your info. I'll study the RCU self-test program rcutorture later.
> 
>>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>
>> 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.
> 
> Oh, Sorry. It seems that I forgot to delete your Signed-off-by in v2.
> Oops! you reminded me once before. After v1, you helped modify the
> description and pull it into your tree. I got it from 'dev' branch.
> 
>>
>> So were you looking for me to take these two patches?
> 
> Yes, it could be quicker. Of course, I can wait for patch 1/2 upstream,
> then repost patch 2/2. In fact, I also want to dump part of the slab
> object, I've already written the code. In order not to affect the current
> user of mem_dump_obj(), a new parameter need to be added to kmem_dump_obj().
> I will post v5 with this patch later.

I measured it carefully, and the code would look ugly when I added a parameter.
In fact, the three places where mem_dump_obj() is currently called are for
debugging purposes, and dump the memory of slab object is not bad for them.

> 
>>
>> 							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
>>>
>> .
>>
> 

-- 
Regards,
  Zhen Lei