On Wed, 23 Aug 2023 at 17:05, Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx> wrote: > > Currently, xsk_build_skb() is a function that builds skb in two possible > ways and then is ended with common error handling. > > We can distinguish four possible error paths and handling in xsk_build_skb(): > 1. sock_alloc_send_skb fails: Retry (skb is NULL). > 2. skb_store_bits fails : Free skb and retry. > 3. MAX_SKB_FRAGS exceeded: Free skb, cleanup and drop packet. > 4. alloc_page fails for frag: Retry page allocation w/o freeing skb > > 1] and 3] can happen in xsk_build_skb_zerocopy(), which is one of the two > code paths responsible for building skb. Common error path in > xsk_build_skb() assumes that in case errno != -EAGAIN, skb is a valid > pointer, which is wrong as kernel test robot reports that in > xsk_build_skb_zerocopy() other errno values are returned for skb being > NULL. > > To fix this, set -EOVERFLOW as error when MAX_SKB_FRAGS are exceeded and > packet needs to be dropped in both xsk_build_skb() and > xsk_build_skb_zerocopy() and use this to distinguish against all other > error cases. Also, add explicit kfree_skb() for 3] so that handling of 1], > 2], and 3] becomes identical where allocation needs to be retried. Thanks Tirtha for the fix. Acked-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx> > 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: > v2 -> v3: > - Added further details in commit message as asked by Maciej > 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) { > + /* Drop the packet */ > + xsk_set_destructor_arg(xs->skb); > + xsk_drop_skb(xs->skb); > xskq_cons_release(xs->tx); > + } else { > + /* Let application retry */ > + xsk_cq_cancel_locked(xs, 1); > } > > return ERR_PTR(err); > @@ -738,7 +742,7 @@ static int __xsk_generic_xmit(struct sock *sk) > skb = xsk_build_skb(xs, &desc); > if (IS_ERR(skb)) { > err = PTR_ERR(skb); > - if (err == -EAGAIN) > + if (err != -EOVERFLOW) > goto out; > err = 0; > continue; > -- > 2.34.1 > >
