linux-kernel - Re: [PATCH v12 2/4] mm/slub: allow to set node and align in k[v]realloc

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

Message-ID: <CAH5fLggV2Y+N7k=+b5z=Hy5a556VomEZEa3bGhufvKBU+Fd=Ug@mail.gmail.com>
Date: Tue, 15 Jul 2025 09:03:27 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Vitaly Wool <vitaly.wool@...sulko.se>, Harry Yoo <harry.yoo@...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>, rust-for-linux@...r.kernel.org, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>, 
	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 2/4] mm/slub: allow to set node and align in k[v]realloc

On Mon, Jul 14, 2025 at 10:14 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 7/12/25 14:43, Vitaly Wool wrote:
> >
> >
> >> On Jul 11, 2025, at 5:43 PM, Vlastimil Babka <vbabka@...e.cz> wrote:
> >>
> >> On 7/11/25 10:58, Harry Yoo wrote:
> >>> On Wed, Jul 09, 2025 at 07:24:41PM +0200, Vitaly Wool wrote:
> >>>> static __always_inline __realloc_size(2) void *
> >>>> -__do_krealloc(const void *p, size_t new_size, gfp_t flags)
> >>>> +__do_krealloc(const void *p, size_t new_size, unsigned long align, gfp_t flags, int nid)
> >>>> {
> >>>> void *ret;
> >>>> size_t ks = 0;
> >>>> @@ -4859,6 +4859,20 @@ __do_krealloc(const void *p, size_t new_size, gfp_t flags)
> >>>> if (!kasan_check_byte(p))
> >>>> return NULL;
> >>>>
> >>>> + /* refuse to proceed if alignment is bigger than what kmalloc() provides */
> >>>> + if (!IS_ALIGNED((unsigned long)p, align) || new_size < align)
> >>>> + return NULL;
> >>>
> >>> Hmm but what happens if `p` is aligned to `align`, but the new object is not?
> >>>
> >>> For example, what will happen if we  allocate object with size=64, align=64
> >>> and then do krealloc with size=96, align=64...
> >>>
> >>> Or am I missing something?
> >>
> >> Good point. We extended the alignment guarantees in commit ad59baa31695
> >> ("slab, rust: extend kmalloc() alignment guarantees to remove Rust padding")
> >> for rust in a way that size 96 gives you alignment of 32. It assumes that
> >> rust side will ask for alignments that are power-of-two and sizes that are
> >> multiples of alignment. I think if that assumption is still honored than
> >> this will keep working, but the check added above (is it just a sanity check
> >> or something the rust side relies on?) doesn't seem correct?
> >>
> >
> > It is a sanity check and it should have looked like this:
> >
> >         if (!IS_ALIGNED((unsigned long)p, align) && new_size <= ks)
> >                 return NULL;
> >
> > and the reasoning for this is the following: if we don’t intend to reallocate (new size is not bigger than the original size), but the user requests a larger alignment, it’s a miss. Does that sound reasonable?
>
> So taking a step back indeed the align passed to krealloc is indeed used
> only for this check. If it's really just a sanity check, then I'd rather not
> add this parameter to krealloc functions at all - kmalloc() itself also
> doesn't have it, so it's inconsistent that krealloc() would have it - but
> only to return NULL and not e.g. try to reallocate for alignment.
>
> If it's not just a sanity check, it means it's expected that for some
> sequence of valid kvrealloc_node_align() calls it can return NULL and then
> rely on the fallback to vmalloc. That would be rather wasteful for the cases
> like going from 64 to 96 bytes etc. So in that case it would be better if
> krealloc did the reallocation, same as in cases when size increases. Of
> course it would still have to rely on the documented alignment guarantees
> only and not provide anything arbitrary. aligned_size() in
> rust/kernel/alloc/allocator.rs is responsible for that, AFAIK.
>
> And I think it's not a sanity check but the latter - if the following is a
> valid k(v)realloc sequence (from Rust POV). The individual size+align
> combinations AFAIK are, but if it's valid to make them follow one another
> them like this, I don't know.
>
> krealloc(size=96, align=32) -> can give object with 32 alignment only
> krealloc(size=64, align=64) -> doesn't increase size but wants alignment 64

On the Rust side, you specify what the old size/align was, and what
you which size/align you want after the call, and they can be anything
including that combination.

Alice