linux-kernel - Re: [PATCH] mm/memmap: Prevent double scanning of memmap by kmemleak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOm6qnnp64pQcMQgUf67iBHX7o3H+gQ8iRUemhENiccakN1gBA@mail.gmail.com>
Date: Fri, 3 Jan 2025 08:01:24 +0800
From: Weikang Guo <guoweikang.kernel@...il.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: Mike Rapoport <rppt@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/memmap: Prevent double scanning of memmap by kmemleak

Hi,Catalin

Catalin Marinas <catalin.marinas@....com> wrote on Friday, 3 January 2025 01:15:
>
> On Thu, Jan 02, 2025 at 02:57:03PM +0800, Guo Weikang wrote:
> > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > index 673d5cae7c81..b0483c534ef7 100644
> > --- a/include/linux/memblock.h
> > +++ b/include/linux/memblock.h
> > @@ -375,7 +375,13 @@ static inline int memblock_get_region_node(const struct memblock_region *r)
> >  }
> >  #endif /* CONFIG_NUMA */
> >
> > -/* Flags for memblock allocation APIs */
> > +/*
> > + * Flags for memblock allocation APIs
> > + * MEMBLOCK_ALLOC_ANYWHERE and MEMBLOCK_ALLOC_ACCESSIBLE
> > + * indicates wheather the allocation is limited by memblock.current_limit.
> > + * MEMBLOCK_ALLOC_NOLEAKTRACE not only indicates that it does not need to
> > + * be scanned by kmemleak, but also implies MEMBLOCK_ALLOC_ACCESSIBLE
> > + */
>
> I'd keep the comment short here, something like:
>
> /*
>  * MEMBLOCK_ALLOC_NOLEAKTRACE avoids kmemleak tracing. It implies
>  * MEMBLOCK_ALLOC_ACCESSIBLE.
>  */
>

Thanks, the previous version would be a bit redundant.

> > -/*
> > - * Allocate a block of memory to be used to back the virtual memory map
> > - * or to back the page tables that are used to create the mapping.
> > - * Uses the main allocators if they are available, else bootmem.
> > - */
> > -
> > -static void * __ref __earlyonly_bootmem_alloc(int node,
> > -                             unsigned long size,
> > -                             unsigned long align,
> > -                             unsigned long goal)
> > -{
> > -     return memblock_alloc_try_nid_raw(size, align, goal,
> > -                                            MEMBLOCK_ALLOC_ACCESSIBLE, node);
> > -}
> > -
> >  void * __meminit vmemmap_alloc_block(unsigned long size, int node)
> >  {
> >       /* If the main allocator is up use that, fallback to bootmem. */
> > @@ -66,8 +51,7 @@ void * __meminit vmemmap_alloc_block(unsigned long size, int node)
> >               }
> >               return NULL;
> >       } else
> > -             return __earlyonly_bootmem_alloc(node, size, size,
> > -                             __pa(MAX_DMA_ADDRESS));
> > +             return memmap_alloc(size, size, __pa(MAX_DMA_ADDRESS), node, false);
> >  }
>
> As the kernel test robot reported, the __ref annotation for
> __earlyonly_bootmem_alloc() is still needed, otherwise you get a warning
> that a __meminit function (vmemmap_alloc_block()) is calling an __init
> one (memmap_alloc()). So I think it's better if you keep this function.
> Maybe get it to call memmap_alloc() instead of
> memblock_alloc_try_nid_raw().
>

I have also noticed and studied the previous implementation, it is clear to me
that _ref annotation is used to prevent init section mismatch WARNING.
I will recover __earlyonly_bootmem_alloc().

> --
> Catalin

Thanks a lot

--
Guo