netdev - Re: [PATCH net-next] net: shrink napi_skb_cache

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: <73aeafc5-75eb-42dc-8f26-ca54dc7506da@intel.com>
Date: Thu, 16 Oct 2025 13:07:57 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
	<horms@...nel.org>, Kuniyuki Iwashima <kuniyu@...gle.com>,
	<netdev@...r.kernel.org>, <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next] net: shrink napi_skb_cache_put()

From: Eric Dumazet <edumazet@...gle.com>
Date: Thu, 16 Oct 2025 03:31:37 -0700

> On Thu, Oct 16, 2025 at 3:20 AM Alexander Lobakin
> <aleksander.lobakin@...el.com> wrote:
>>
>> From: Eric Dumazet <edumazet@...gle.com>
>> Date: Wed, 15 Oct 2025 23:38:01 +0000
>>
>>> Following loop in napi_skb_cache_put() is unrolled by the compiler
>>> even if CONFIG_KASAN is not enabled:
>>>
>>> for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++)
>>>       kasan_mempool_unpoison_object(nc->skb_cache[i],
>>>                               kmem_cache_size(net_hotdata.skbuff_cache));
>>>
>>> We have 32 times this sequence, for a total of 384 bytes.
>>>
>>>       48 8b 3d 00 00 00 00    net_hotdata.skbuff_cache,%rdi
>>>       e8 00 00 00 00          call   kmem_cache_size
>>>
>>> This is because kmem_cache_size() is an extern function,
>>> and kasan_unpoison_object_data() is an inline function.
>>>
>>> Cache kmem_cache_size() result in a temporary variable, and
>>> make the loop conditional to CONFIG_KASAN.
>>>
>>> After this patch, napi_skb_cache_put() is inlined in its callers.
>>>
>>> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>>> Cc: Alexander Lobakin <aleksander.lobakin@...el.com>
>>> ---
>>>  net/core/skbuff.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..5a8b48b201843f94b5fdaab3241801f642fbd1f0 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -1426,10 +1426,13 @@ static void napi_skb_cache_put(struct sk_buff *skb)
>>>       nc->skb_cache[nc->skb_count++] = skb;
>>>
>>>       if (unlikely(nc->skb_count == NAPI_SKB_CACHE_SIZE)) {
>>> -             for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++)
>>> -                     kasan_mempool_unpoison_object(nc->skb_cache[i],
>>> -                                             kmem_cache_size(net_hotdata.skbuff_cache));
>>> +             if (IS_ENABLED(CONFIG_KASAN)) {
>>> +                     u32 size = kmem_cache_size(net_hotdata.skbuff_cache);
>>>
>>> +                     for (i = NAPI_SKB_CACHE_HALF; i < NAPI_SKB_CACHE_SIZE; i++)
>>> +                             kasan_mempool_unpoison_object(nc->skb_cache[i],
>>> +                                                           size);
>>> +             }
>>
>> Very interesting; back when implementing napi_skb_cache*() family and
>> someone (most likely Jakub) asked me to add KASAN-related checks here,
>> I was comparing the object code and stopped on the current variant, as
>> without KASAN, the entire loop got optimized away (but only when
>> kmem_cache_size() is *not* a temporary variable).
>>
>> Or does this patch addresses KASAN-enabled kernels? Either way, if this
>> patch really optimizes things:
> 
> No, this is when CONFIG_KASAN is _not_ enabled.
> 
> (I have not checked when it is enabled, I do not care about the cost
> of KASAN as long as it is not too expensive)
> 
> Compiler does not know anything about kmem_cache_size()
> It could contain some memory cloberring, memory freeing, some kind of
> destructive action.
> 
> So it has to call it 32 times.
> 
> And reload net_hotdata.skbuff_cache 32 times, because the value could
> have been changed
> by kmem_cache_size() (if kmem_cache_size() wanted to)
> 
> Not sure if kmem_cache_size() could be inlined.

BTW doesn't napi_skb_cache_get() (inc. get_bulk()) suffer the same way?

> 
> Its use has been discouraged so I guess nobody cared.
> 
>>
>> Acked-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>>
>>>               kmem_cache_free_bulk(net_hotdata.skbuff_cache, NAPI_SKB_CACHE_HALF,
>>>                                    nc->skb_cache + NAPI_SKB_CACHE_HALF);
>>>               nc->skb_count = NAPI_SKB_CACHE_HALF;
Thanks,
Olek