[PATCH v2] mm/memmap: Prevent double scanning of memmap by kmemleak

[PATCH v2] mm/memmap: Prevent double scanning of memmap by kmemleak

[Guo Weikang]

kmemleak explicitly scans the mem_map through the valid struct page objects.
However, memmap_alloc() was also adding this memory to the gray object list,
causing it to be scanned twice. Removes memmap_alloc() from the
scan list and adds a comment to clarify the behavior.

Link: https://lore.kernel.org/lkml/CAOm6qn=FVeTpH54wGDFMHuCOeYtvoTx30ktnv9-w3Nh8RMofEA@mail.gmail.com/

Signed-off-by: Guo Weikang <guoweikang.kernel@gmail.com>
---
v1 -> v2: Fix CI: Recover __earlyonly_bootmem_alloc that is marked with
__ref to prevent init section mismatch WARNING.
Links:  https://lore.kernel.org/oe-kbuild-all/202501021601.jub4p3EM-lkp@intel.com/
---
 include/linux/memblock.h | 4 ++++
 mm/mm_init.c             | 8 ++++++--
 mm/sparse-vmemmap.c      | 5 ++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index dee628350cd1..e79eb6ac516f 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -378,6 +378,10 @@ static inline int memblock_get_region_node(const struct memblock_region *r)
 /* Flags for memblock allocation APIs */
 #define MEMBLOCK_ALLOC_ANYWHERE	(~(phys_addr_t)0)
 #define MEMBLOCK_ALLOC_ACCESSIBLE	0
+/*
+ *  MEMBLOCK_ALLOC_NOLEAKTRACE avoids kmemleak tracing. It implies
+ *  MEMBLOCK_ALLOC_ACCESSIBLE
+ */
 #define MEMBLOCK_ALLOC_NOLEAKTRACE	1
 
 /* We are using top down, so it is safe to use 0 here */
diff --git a/mm/mm_init.c b/mm/mm_init.c
index 24b68b425afb..71b58f5f2492 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
 	}
 }
 
+/*
+ * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`,
+ * so memblock does not need to be added to the scan list.
+ */
 void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
 			  phys_addr_t min_addr, int nid, bool exact_nid)
 {
@@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
 
 	if (exact_nid)
 		ptr = memblock_alloc_exact_nid_raw(size, align, min_addr,
-						   MEMBLOCK_ALLOC_ACCESSIBLE,
+						   MEMBLOCK_ALLOC_NOLEAKTRACE,
 						   nid);
 	else
 		ptr = memblock_alloc_try_nid_raw(size, align, min_addr,
-						 MEMBLOCK_ALLOC_ACCESSIBLE,
+						 MEMBLOCK_ALLOC_NOLEAKTRACE,
 						 nid);
 
 	if (ptr && size > 0)
diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index cec67c5f37d8..903a5422907b 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -27,7 +27,7 @@
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
-
+#include "internal.h"
 #include <asm/dma.h>
 #include <asm/pgalloc.h>
 
@@ -42,8 +42,7 @@ static void * __ref __earlyonly_bootmem_alloc(int node,
 				unsigned long align,
 				unsigned long goal)
 {
-	return memblock_alloc_try_nid_raw(size, align, goal,
-					       MEMBLOCK_ALLOC_ACCESSIBLE, node);
+	return memmap_alloc(size, align, goal, node, false);
 }
 
 void * __meminit vmemmap_alloc_block(unsigned long size, int node)
-- 
2.25.1

Re: [PATCH v2] mm/memmap: Prevent double scanning of memmap by kmemleak

[Catalin Marinas]

On Fri, Jan 03, 2025 at 07:01:50PM +0800, Guo Weikang wrote:
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index 24b68b425afb..71b58f5f2492 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
>  	}
>  }
>  
> +/*
> + * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`,
> + * so memblock does not need to be added to the scan list.
> + */
>  void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
>  			  phys_addr_t min_addr, int nid, bool exact_nid)
>  {

Nitpick: normally I'd place the comment in the code, before the 'if'
statement. We keep the comments above functions for a description of
the function.

> @@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
>  
>  	if (exact_nid)
>  		ptr = memblock_alloc_exact_nid_raw(size, align, min_addr,
> -						   MEMBLOCK_ALLOC_ACCESSIBLE,
> +						   MEMBLOCK_ALLOC_NOLEAKTRACE,
>  						   nid);
>  	else
>  		ptr = memblock_alloc_try_nid_raw(size, align, min_addr,
> -						 MEMBLOCK_ALLOC_ACCESSIBLE,
> +						 MEMBLOCK_ALLOC_NOLEAKTRACE,
>  						 nid);
>  
>  	if (ptr && size > 0)
> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index cec67c5f37d8..903a5422907b 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c
> @@ -27,7 +27,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/vmalloc.h>
>  #include <linux/sched.h>
> -
> +#include "internal.h"
>  #include <asm/dma.h>
>  #include <asm/pgalloc.h>

Another nit: if the original authors of this file preferred the includes
to be in blocks, I'd keep the internal.h include as a separate block
after the asm/ includes.

Apart from these minor things, the patch looks fine.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks.

-- 
Catalin

Re: [PATCH v2] mm/memmap: Prevent double scanning of memmap by kmemleak

[Weikang Guo]

Thansk， Catalin

Catalin Marinas <catalin.marinas@arm.com> wrote on Friday, 3 January
2025 at 22:28
>
> On Fri, Jan 03, 2025 at 07:01:50PM +0800, Guo Weikang wrote:
> > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > index 24b68b425afb..71b58f5f2492 100644
> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
> >       }
> >  }
> >
> > +/*
> > + * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`,
> > + * so memblock does not need to be added to the scan list.
> > + */
> >  void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
> >                         phys_addr_t min_addr, int nid, bool exact_nid)
> >  {
>
> Nitpick: normally I'd place the comment in the code, before the 'if'
> statement. We keep the comments above functions for a description of
> the function.
>

Thank you for the suggestion! I agree that placing the comment before
the 'if' statement is
a better approach. I will update the patch accordingly.

> > @@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align,
> >
> >       if (exact_nid)
> >               ptr = memblock_alloc_exact_nid_raw(size, align, min_addr,
> > -                                                MEMBLOCK_ALLOC_ACCESSIBLE,
> > +                                                MEMBLOCK_ALLOC_NOLEAKTRACE,
> >                                                  nid);
> >       else
> >               ptr = memblock_alloc_try_nid_raw(size, align, min_addr,
> > -                                              MEMBLOCK_ALLOC_ACCESSIBLE,
> > +                                              MEMBLOCK_ALLOC_NOLEAKTRACE,
> >                                                nid);
> >
> >       if (ptr && size > 0)
> > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> > index cec67c5f37d8..903a5422907b 100644
> > --- a/mm/sparse-vmemmap.c
> > +++ b/mm/sparse-vmemmap.c
> > @@ -27,7 +27,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/sched.h>
> > -
> > +#include "internal.h"
> >  #include <asm/dma.h>
> >  #include <asm/pgalloc.h>
>
> Another nit: if the original authors of this file preferred the includes
> to be in blocks, I'd keep the internal.h include as a separate block
> after the asm/ includes.
>

Here is the order I actually refer to `mm/sparse.c`, but after looking at
other files, I think your suggestion is better.

> Apart from these minor things, the patch looks fine.
>

Tanks!

> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Thanks.
>
> --
> Catalin


---
Guo