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

Message-ID: <aLHR4qMphDdG43l2@boxer>
Date: Fri, 29 Aug 2025 18:14:26 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Daniel Borkmann <daniel@...earbox.net>
CC: 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 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 will now send a v7 which is supposed to address the reported perf impact
by Jason...keep your fingers crossed for me not messing anything again
around this code base:D

> 
> > regards,
> > dan carpenter
> > 
> > > +			return ERR_PTR(-ENOMEM);
> > > +		}
> > > +
> > > +		xsk_set_destructor_arg(skb, addrs);
> > >   	}
> > >   	addr = desc->addr;
> > 
>