netdev - Re: [PATCH v6 bpf] xsk: fix immature cq descriptor production

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] [day] [month] [year] [list]

Message-ID: <CAL+tcoD1t4a5wEUYKJbhFHC2Cf8smY983JzWgLhjpcHd7-fwGg@mail.gmail.com>
Date: Sat, 30 Aug 2025 18:32:32 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: Daniel Borkmann <daniel@...earbox.net>, Dan Carpenter <dan.carpenter@...aro.org>, bpf@...r.kernel.org, 
	ast@...nel.org, andrii@...nel.org, netdev@...r.kernel.org, 
	magnus.karlsson@...el.com, stfomichev@...il.com, aleksander.lobakin@...el.com, 
	Eryk Kubanski <e.kubanski@...tner.samsung.com>
Subject: Re: [PATCH v6 bpf] xsk: fix immature cq descriptor production

On Sat, Aug 30, 2025 at 12:15 AM Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> On Tue, Aug 26, 2025 at 02:23:34PM +0200, Daniel Borkmann wrote:
> > On 8/26/25 10:14 AM, Dan Carpenter wrote:
> > > On Wed, Aug 20, 2025 at 05:44:16PM +0200, Maciej Fijalkowski wrote:
> > > >                           return ERR_PTR(err);
> > > >                   skb_reserve(skb, hr);
> > > > +
> > > > +         addrs = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
> > > > +         if (!addrs) {
> > > > +                 kfree(skb);
> > >
> > > This needs to be kfree_skb(skb);
> >
> > Oh well, good catch! Maciej, given this commit did not land yet in Linus' tree,
> > I can toss the commit from bpf tree assuming you send a v7?
> >
> > Also, looking at xsk_build_skb(), do we similarly need to free that allocated
> > skb when we hit the ERR_PTR(-EOVERFLOW) ? Mentioned function has the following
> > in the free_err path:
> >
> >         if (first_frag && skb)
> >                 kfree_skb(skb);
> >
> > Pls double check.
>
> for EOVERFLOW we drop skb and then we continue with consuming next
> descriptors from XSK Tx queue. Every other errno causes this loop
> processing to stop and give the control back to application side. skb
> pointer is kept within xdp_sock and on next syscall we will retry with
> sending it.
>
>         if (err == -EOVERFLOW) {
>                 xsk_drop_skb(xs->skb);
>                 -> xsk_consume_skb(skb);
>                    -> consume_skb(skb);
>
> since it's a drop, i wonder if we should have a kfree_skb() with proper
> drop reason for XSK subsystem, but that's for a different discussion.

I agree on this point. A few days ago, I scanned the code over and
over again and stumbled on this. Sure, we can add reasons into it.

Thanks,
Jason