On Sat, Aug 30, 2025 at 12:15 AM Maciej Fijalkowski
<maciej.fijalkowski@xxxxxxxxx> 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
