linux-kernel - Re: [PATCH v12 1/4] mm/vmalloc: allow to set node and align in vrealloc

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-next>] [day] [month] [year] [list]

Message-ID: <aedc2b36-b3b0-4367-aa68-ba9f8a110b52@lucifer.local>
Date: Thu, 10 Jul 2025 16:19:40 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vitaly Wool <vitaly.wool@...sulko.se>
Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>, linux-mm@...ck.org,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Uladzislau Rezki <urezki@...il.com>,
        Danilo Krummrich <dakr@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, rust-for-linux@...r.kernel.org,
        Kent Overstreet <kent.overstreet@...ux.dev>,
        linux-bcachefs@...r.kernel.org, bpf@...r.kernel.org,
        Herbert Xu <herbert@...dor.apana.org.au>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>
Subject: Re: [PATCH v12 1/4] mm/vmalloc: allow to set node and align in
 vrealloc

On Thu, Jul 10, 2025 at 08:21:19AM +0200, Vitaly Wool wrote:
>
>
> > On Jul 9, 2025, at 9:01 PM, Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
> >
> > * Vitaly Wool <vitaly.wool@...sulko.se <mailto:vitaly.wool@...sulko.se>> [250709 13:24]:
> >> Reimplement vrealloc() to be able to set node and alignment should
> >> a user need to do so. Rename the function to vrealloc_node_align()
> >> to better match what it actually does now and introduce macros for
> >> vrealloc() and friends for backward compatibility.
> >>
> >> With that change we also provide the ability for the Rust part of
> >> the kernel to set node and alignment in its allocations.
> >>
> >> Signed-off-by: Vitaly Wool <vitaly.wool@...sulko.se>
> >> Reviewed-by: Uladzislau Rezki (Sony) <urezki@...il.com>
> >> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
> >> ---
> >> include/linux/vmalloc.h | 12 +++++++++---
> >> mm/nommu.c              |  3 ++-
> >> mm/vmalloc.c            | 31 ++++++++++++++++++++++++++-----
> >> 3 files changed, 37 insertions(+), 9 deletions(-)
> >>
> > ...
> >
> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >> index 6dbcdceecae1..03dd06097b25 100644
> >> --- a/mm/vmalloc.c
> >> +++ b/mm/vmalloc.c
> >> @@ -4089,19 +4089,31 @@ void *vzalloc_node_noprof(unsigned long size, int node)
> >> EXPORT_SYMBOL(vzalloc_node_noprof);
> >>
> >> /**
> >> - * vrealloc - reallocate virtually contiguous memory; contents remain unchanged
> >> + * vrealloc_node_align_noprof - reallocate virtually contiguous memory; contents
> >> + * remain unchanged
> >>  * @p: object to reallocate memory for
> >>  * @size: the size to reallocate
> >> + * @align: requested alignment
> >>  * @flags: the flags for the page level allocator
> >> + * @nid: node number of the target node
> >> + *
> >> + * If @p is %NULL, vrealloc_XXX() behaves exactly like vmalloc(). If @size is
> >> + * 0 and @p is not a %NULL pointer, the object pointed to is freed.
> >>  *
> >> - * If @p is %NULL, vrealloc() behaves exactly like vmalloc(). If @size is 0 and
> >> - * @p is not a %NULL pointer, the object pointed to is freed.
> >> + * if @nid is not NUMA_NO_NODE, this function will try to allocate memory on
> >> + * the given node. If reallocation is not necessary (e. g. the new size is less
> >> + * than the current allocated size), the current allocation will be preserved
> >> + * unless __GFP_THISNODE is set. In the latter case a new allocation on the
> >> + * requested node will be attempted.

Agreed with Liam, this is completely unreadable.

I think the numa node stuff is unnecesasry, that's pretty much inferred.

I'd just go with something like 'if the function can void having to reallocate
then it does'.

Nice and simple :)

> >
> > I am having a very hard time understanding what you mean here.  What is
> > the latter case?
> >
> > If @nis is !NUMA_NO_NODE, the allocation will be attempted on the given
> > node.  Then things sort of get confusing.  What is the latter case?
>
> The latter case is __GFP_THISNODE present in flags. That’s the latest if-clause in this paragraph.
> >
> >>  *
> >>  * If __GFP_ZERO logic is requested, callers must ensure that, starting with the
> >>  * initial memory allocation, every subsequent call to this API for the same
> >>  * memory allocation is flagged with __GFP_ZERO. Otherwise, it is possible that
> >>  * __GFP_ZERO is not fully honored by this API.
> >>  *
> >> + * If the requested alignment is bigger than the one the *existing* allocation
> >> + * has, this function will fail.
> >> + *
> >
> > It might be better to say something like:
> > Requesting an alignment that is bigger than the alignment of the
> > *existing* allocation will fail.
> >
>
> The whole function description in fact consists of several if-clauses (some of which are nested) so I am just following the pattern here.

Right, but in no sane world is essentially describing a series of if-clauses in
a kerneldoc a thing.

Just it keep it simple, this is meant to be an overview, people can go read the
code if they need details :)

>
> ~Vitaly

Cheers, Lorenzo