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: <CANn89i+birOC7FA9sVtGQNxqQvOGgrY3ychNns7g-uEdOu5p5w@mail.gmail.com>
Date: Thu, 16 Oct 2025 03:31:37 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Alexander Lobakin <aleksander.lobakin@...el.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()

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.

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