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

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

[thunder.leizhen]

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

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: Provide empty function for kmem_dump_obj() when CONFIG_PRINTK=n
  rcu: Dump memory object info if callback function is invalid

 include/linux/slab.h  | 3 +++
 kernel/rcu/rcu.h      | 9 +++++++++
 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 +
 7 files changed, 17 insertions(+)

-- 
2.34.1

[PATCH v3 1/2] mm: Provide empty function for kmem_dump_obj() when CONFIG_PRINTK=n

[thunder.leizhen]

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

Commit 5bb1bb353cfe ("mm: Don't build mm_dump_obj() on CONFIG_PRINTK=n
kernels") only provides static inline empty function for mem_dump_obj().
But functions kmem_valid_obj() and kmem_dump_obj() are also exported,
they may be called by functions other than mem_dump_obj(), so their empty
functions should also be provided when CONFIG_PRINTK=n.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/slab.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 848c7c82ad5ad0b..fc05fe288d176b4 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -246,6 +246,9 @@ size_t ksize(const void *objp);
 #ifdef CONFIG_PRINTK
 bool kmem_valid_obj(void *object);
 void kmem_dump_obj(void *object);
+#else
+static inline bool kmem_valid_obj(void *object) { return false; }
+static inline void kmem_dump_obj(void *object) {}
 #endif
 
 /*
-- 
2.34.1

Re: [PATCH v3 1/2] mm: Provide empty function for kmem_dump_obj() when CONFIG_PRINTK=n

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 11:45:16AM +0800, thunder.leizhen@huaweicloud.com wrote:
> +++ b/include/linux/slab.h
> @@ -246,6 +246,9 @@ size_t ksize(const void *objp);
>  #ifdef CONFIG_PRINTK
>  bool kmem_valid_obj(void *object);
>  void kmem_dump_obj(void *object);
> +#else
> +static inline bool kmem_valid_obj(void *object) { return false; }

That is very confusing.  kmem_valid_obj() looks like a function which
should exist regardless of CONFIG_PRINTK and to have it always return
false if CONFIG_PRINTK isn't set seems weird.

I see we have one caller of kmem_valid_obj() right now.  Which means it
shouldn't be an EXPORT_SYMBOL since that caller is not a module.

I think the right solution is to convert kmem_dump_obj() to
work the same way as vmalloc_dump_obj().  ie:

+++ b/mm/util.c
@@ -1057,11 +1057,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;

... with corresponding changes to eliminate kmem_valid_obj() as a
symbol.

Re: [PATCH v3 1/2] mm: Provide empty function for kmem_dump_obj() when CONFIG_PRINTK=n

[Leizhen (ThunderTown)]

On 2023/8/2 11:57, Matthew Wilcox wrote:
> On Wed, Aug 02, 2023 at 11:45:16AM +0800, thunder.leizhen@huaweicloud.com wrote:
>> +++ b/include/linux/slab.h
>> @@ -246,6 +246,9 @@ size_t ksize(const void *objp);
>>  #ifdef CONFIG_PRINTK
>>  bool kmem_valid_obj(void *object);
>>  void kmem_dump_obj(void *object);
>> +#else
>> +static inline bool kmem_valid_obj(void *object) { return false; }
> 
> That is very confusing.  kmem_valid_obj() looks like a function which
> should exist regardless of CONFIG_PRINTK and to have it always return
> false if CONFIG_PRINTK isn't set seems weird.

Yes, I noticed it, but I didn't come up with a good idea.

> 
> I see we have one caller of kmem_valid_obj() right now.  Which means it
> shouldn't be an EXPORT_SYMBOL since that caller is not a module.
> 
> I think the right solution is to convert kmem_dump_obj() to
> work the same way as vmalloc_dump_obj().  ie:

Okay, it's a good suggestion.

In fact, kmem_dump_obj() also does what kmem_valid_obj() does, except
that it will print warning if the check fails. So, do as you suggest,
the duplicated code can be eliminated.

> 
> +++ b/mm/util.c
> @@ -1057,11 +1057,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;
> 
> ... with corresponding changes to eliminate kmem_valid_obj() as a
> symbol.
> 
> .
> 

-- 
Regards,
  Zhen Lei

[PATCH v3 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      | 9 +++++++++
 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, 14 insertions(+)

diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index d1dcb09750efbd6..30a7b0a79ae7a67 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/mm.h>
 #include <trace/events/rcu.h>
 
 /*
@@ -248,6 +249,14 @@ 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)) {
+		if (kmem_valid_obj(rhp))
+			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 v3 2/2] rcu: Dump memory object info if callback function is invalid

[Matthew Wilcox]

On Wed, Aug 02, 2023 at 11:45:17AM +0800, thunder.leizhen@huaweicloud.com wrote:
> +#include <linux/mm.h>

mm.h is a very complex file with many dependencies.  Don't you really
only need slab.h?

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

[Leizhen (ThunderTown)]

On 2023/8/2 11:48, Matthew Wilcox wrote:
> On Wed, Aug 02, 2023 at 11:45:17AM +0800, thunder.leizhen@huaweicloud.com wrote:
>> +#include <linux/mm.h>
> 
> mm.h is a very complex file with many dependencies.  Don't you really
> only need slab.h?

Oh, yes, thanks.

> 
> .
> 

-- 
Regards,
  Zhen Lei