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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

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;
> > 
>