> -----Original Message----- > From: Stanislav Fomichev <sdf@xxxxxxxxxx> > Sent: Tuesday, August 15, 2023 11:51 PM > On 08/15, Tirthendu Sarkar wrote: > > xsk_build_skb_zerocopy() may return an error other than -EAGAIN and > this > > is received as skb and used later in xsk_set_destructor_arg() and > > xsk_drop_skb() which must operate on a valid skb. > > > > Set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and > packet needs > > to be dropped and use this to distinguish against all other error cases > > where allocation needs to be retried. > > > > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > Closes: https://lore.kernel.org/r/202307210434.OjgqFcbB-lkp@xxxxxxxxx/ > > Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx > path") > > > > Changelog: > > v1 -> v2: > > - Removed err as a parameter to xsk_build_skb_zerocopy() > > [Stanislav Fomichev] > > - use explicit error to distinguish packet drop vs retry > > --- > > net/xdp/xsk.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c > > index fcfc8472f73d..55f8b9b0e06d 100644 > > --- a/net/xdp/xsk.c > > +++ b/net/xdp/xsk.c > > @@ -602,7 +602,7 @@ static struct sk_buff > *xsk_build_skb_zerocopy(struct xdp_sock *xs, > > > > for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) { > > if (unlikely(i >= MAX_SKB_FRAGS)) > > - return ERR_PTR(-EFAULT); > > + return ERR_PTR(-EOVERFLOW); > > > > page = pool->umem->pgs[addr >> PAGE_SHIFT]; > > get_page(page); > > @@ -655,15 +655,17 @@ static struct sk_buff *xsk_build_skb(struct > xdp_sock *xs, > > skb_put(skb, len); > > > > err = skb_store_bits(skb, 0, buffer, len); > > - if (unlikely(err)) > > + if (unlikely(err)) { > > + kfree_skb(skb); > > goto free_err; > > + } > > } else { > > int nr_frags = skb_shinfo(skb)->nr_frags; > > struct page *page; > > u8 *vaddr; > > > > if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && > xp_mb_desc(desc))) { > > - err = -EFAULT; > > + err = -EOVERFLOW; > > goto free_err; > > } > > > > @@ -690,12 +692,14 @@ static struct sk_buff *xsk_build_skb(struct > xdp_sock *xs, > > return skb; > > > > free_err: > > - if (err == -EAGAIN) { > > - xsk_cq_cancel_locked(xs, 1); > > - } else { > > - xsk_set_destructor_arg(skb); > > - xsk_drop_skb(skb); > > + if (err == -EOVERFLOW) { > > Don't think this will work? We have some other error paths in xsk_build_skb > that are not -EOVERFLOW that still need kfree_skb, right? > There are 4 possible error paths in xsk_build_skb(): 1. sock_alloc_send_skb: skb is NULL; retry 2. skb_store_bits : free skb and retry 3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet 4. alloc_page fails for frag: retry page allocation for frag w/o freeing skb Of these 1] and 3] can also happen in xsk_build_skb_zerocopy() and the error returned is either -EOVERFLOW or something else and the same error handling needs to be done. > I feel like we are trying to share some state between xsk_build_skb and > xsk_build_skb_zerocopy which we really shouldn't share. So how about > we try to have a separate cleanup path in xsk_build_skb_zerocopy? > The only things that are common between *copy and *zerocopy paths are setting the skb headers (destructor/args, mark, priority) and error handling. While we can potentially split out the paths into independent functions, the same skb header settings and error handling will still need to be duplicated in both functions. > Will something like the following (untested / uncompiled) work instead? > > IOW, ideally, xsk_build_skb should look like: > > if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) { > return xsk_build_skb_zerocopy(xs, desc); > } else { > return xsk_build_skb_copy(xs, desc); > /* ^^ current path that should really be a separate func */ > } >
