On Wed, Aug 16, 2023 at 08:04:52AM +0200, Sarkar, Tirthendu wrote: > > -----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. Please be explicit - say that you're changing this error code in xsk_build_skb_zerocopy() otherwise it's not clear. You're not saying anything about kfree_skb() that is added when skb_store_bits() failed. This code is non-trivial so all of the changes need to be described. Also did we test this patch? I believe it would require us to run xdpsock against virtio net device? > > > > > > 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. That would be helpful to have it included in commit message. > > > 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 */ > > } IMHO this is way out of the scope for a fix. We can think later on about cleaning up these paths. > >
