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 for Android: free password hash cracker in your pocket

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

Message-ID: <1dedcee0-c5a2-47b3-ae13-315ad437ae1a@suse.cz>
Date: Mon, 14 Jul 2025 10:14:06 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Vitaly Wool <vitaly.wool@...sulko.se>
Cc: 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>,
 Alice Ryhl <aliceryhl@...gle.com>, 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 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

> ~Vitaly
>