linux-kernel - Re: [PATCH net-next v1] neighbour: Replace kvzalloc() with kzalloc() when GFP

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: <b4a2bf18-c1ec-4ccd-bed9-671a2fd543a9@suse.cz>
Date: Mon, 17 Feb 2025 19:43:19 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Kohei Enju <enjuk@...zon.com>, edumazet@...gle.com
Cc: davem@...emloft.net, gnaaman@...venets.com, horms@...nel.org,
 joel.granados@...nel.org, kohei.enju@...il.com, kuba@...nel.org,
 kuniyu@...zon.com, linux-kernel@...r.kernel.org, lizetao1@...wei.com,
 netdev@...r.kernel.org, pabeni@...hat.com, cl@...ux.com, penberg@...nel.org,
 rientjes@...gle.com, iamjoonsoo.kim@....com, akpm@...ux-foundation.org,
 roman.gushchin@...ux.dev, 42.hyeyoo@...il.com
Subject: Re: [PATCH net-next v1] neighbour: Replace kvzalloc() with kzalloc()
 when GFP_ATOMIC is specified

On 2/17/25 17:52, Kohei Enju wrote:
> + SLAB ALLOCATOR maintainers and reviewers
> 
>> > From: Kohei Enju <enjuk@...zon.com>
>> > Date: Mon, 17 Feb 2025 01:30:16 +0900
>> > > Replace kvzalloc()/kvfree() with kzalloc()/kfree() when GFP_ATOMIC is
>> > > specified, since kvzalloc() doesn't support non-sleeping allocations such
>> > > as GFP_ATOMIC.
>> > >
>> > > With incompatible gfp flags, kvzalloc() never falls back to the vmalloc
>> > > path and returns immediately after the kmalloc path fails.
>> > > Therefore, using kzalloc() is sufficient in this case.
>> > >
>> > > Fixes: 41b3caa7c076 ("neighbour: Add hlist_node to struct neighbour")
>> >
>> > This commit followed the old hash_buckets allocation, so I'd add
>> >
>> >   Fixes: ab101c553bc1 ("neighbour: use kvzalloc()/kvfree()")
>> >
>> > too.
>> >
>> > Both commits were introduced in v6.13, so there's no difference in terms
>> > of backporting though.
>> >
>> > Also, it would be nice to CC mm maintainers in case they have some
>> > comments.
>> 
>> Oh well, we need to trigger neigh_hash_grow() from a process context,
>> or convert net/core/neighbour.c to modern rhashtable.
> 
> Hi all, thanks for your comments.
> 
> kzalloc() uses page allocator when size is larger than 
> KMALLOC_MAX_CACHE_SIZE, so I think what commit ab101c553bc1 ("neighbour: 
> use kvzalloc()/kvfree()") intended could be achieved by using kzalloc().

Indeed, kzalloc() should be equivalent to pre-ab101c553bc1 code. kvmalloc()
would only be necessary if you need more than order-3 page worth of memory
and don't want it to fail because of fragmentation (but indeed it's not
supported in GFP_ATOMIC context). But since you didn't need such large
allocations before, you probably don't need them now too?

> As mentioned, when using GFP_ATOMIC, kvzalloc() only tries the kmalloc 
> path, since the vmalloc path doesn't support the flag.
> In this case, kvzalloc() is equivalent to kzalloc() in that neither try 
> the vmalloc path, so there is no functional change between this patch and 
> either commit ab101c553bc1 ("neighbour: use kvzalloc()/kvfree()") or 

Agreed.

> commit 41b3caa7c076 ("neighbour: Add hlist_node to struct neighbour").
> 
> Actually there's no real bug in the current code so the Fixes tag was not 
> appropriate. I shall remove the tag.

True, the code is just more clear.

Thanks,
Vlastimil

> Regards,
> Kohei