linux-kernel - Re: [PATCH net-next v2 5/9] tun: use bulk NAPI cache allocation in tun_xdp

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: <20251203084708.FKvfWWxW@linutronix.de>
Date: Wed, 3 Dec 2025 09:47:08 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Jesper Dangaard Brouer <hawk@...nel.org>
Cc: Jon Kohler <jon@...anix.com>, Jason Wang <jasowang@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	John Fastabend <john.fastabend@...il.com>,
	Stanislav Fomichev <sdf@...ichev.me>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_)" <bpf@...r.kernel.org>,
	Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next v2 5/9] tun: use bulk NAPI cache allocation in
 tun_xdp_one

On 2025-12-02 18:32:23 [+0100], Jesper Dangaard Brouer wrote:
> > > > +                       napi_consume_skb(skb, 1);
> > > 
> > > I wonder if this would have any side effects since tun_xdp_one() is
> > > not called by a NAPI.
> > 
> > As far as I can tell, this napi_consume_skb is really just an artifact of
> > how it was named and how it was traditionally used.
> > 
> > Now this is really just a napi_consume_skb within a bh disable/enable
> > section, which should meet the requirements of how that interface
> > should be used (again, AFAICT)
> > 
> 
> Yicks - this sounds super ugly.  Just wrapping napi_consume_skb() in bh
> disable/enable section and then assuming you get the same protection as
> NAPI is really dubious.
> 
> Cc Sebastian as he is trying to cleanup these kind of use-case, to make
> kernel preemption work.

I am actually done with this.

Wrapping napi_consume_skb(, 1) in bh-disable basically does the trick if
called from outside-bh section as long as it is not an IRQ section. The
reason is that the skb-head is cached in a per-CPU cache which accessed
only within softirq/ NAPI context.
So you can "return" skbs in NET_TX and have some around in NET_RX.
Otherwise skb is returned directly to the slab allocator.
If this about skb recycling, you using page_pool might help. This
however also expects NAPI/ BH disabled context.

> > > > @@ -2576,13 +2583,24 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
> > > >                 rcu_read_lock();
> > > >                 bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
> > > > 
> > > > -               for (i = 0; i < n; i++) {
> > > > +               num_skbs = napi_skb_cache_get_bulk(skbs, n);
> > > 
> > > Its document said:
> > > 
> > > """
> > > * Must be called *only* from the BH context.
> > > “"”
> > We’re in a bh_disable section here, is that not good enough?
> 
> Again this feels very ugly and prone to painting ourselves into a
> corner, assuming BH-disabled sections have same protection as NAPI.
> 
> (The napi_skb_cache_get/put function are operating on per CPU arrays
> without any locking.)

This is okay. NAPI means BH is disabled. Nothing more. There are a few
implications to it.
The default path is
 process-context (kernel or userland)
 * IRQ *
   -> irq is handled via its handler with disabled interrupts
   -> handler raises NET_RX aka NAPI
   -> irq core is done with IRQ handling and notices softirqs have been
      raised. Disables BH and starts handling softirqs with enabled
      interrupts before returning back before the interruption.
   -> softirqs are handled with with BH disabled.
   * IRQ * fires again.
     -> irq is handled as previously and NET_RX is set again.
     -> irq core returns back to previously handled softirqs
   -> Once NET_RX is done, softirq core would be done and return back
      but since it noticed that NET_RX is pending (again) it does
      another round.

This is how it normally works. If you disable-bh in process context
(either manually via local_bh_disable() or via spin_lock_bh()) then you
enter BH context. There is hardly a difference (in_serving_softirq()
will report a different value but this should not matter to anyone
outside the core code).
Any IRQ that raises NET_RX here will not lead to handling softirqs
because BH is disabled (this maps the "IRQ fires again" case from
above). This is delayed until local_bh_enable().

Therefore protecting the per-CPU array with local_bh_disable() is okay
but for PREEMPT_RT reasons, per-CPU data needs this
local_lock_nested_bh() around it (as napi_skb_cache_get/put does).

> --Jesper

Sebastian