Re: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xsk: drain continuation descs after overflow in xsk_build_skb()

This patch fixes the AF_XDP generic xmit path so that when a
multi-buffer packet exceeds MAX_SKB_FRAGS or contains an invalid
descriptor, the remaining continuation descriptors are drained onto
the completion ring instead of being silently lost. A new
xdp_sock::drain_cont flag tracks whether draining is in progress
across sendmsg invocations.

Link: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/

> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index ebac60a3d8a1..8b51876efbed 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -80,6 +80,7 @@ struct xdp_sock {
>  	 * call of __xsk_generic_xmit().
>  	 */
>  	struct sk_buff *skb;
> +	bool drain_cont;

[Low]
Would it be worth documenting drain_cont here in the same style as the
preceding skb field? The semantics are non-trivial: the flag persists
across sendmsg invocations, only clears when a non-CONTD descriptor is
consumed, and can be transiently overwritten in the post-loop error
path with the composed expression:

    bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);

Today those semantics can only be reconstructed by reading the four
separate assignment sites in net/xdp/xsk.c. The options field is
also user-influenceable (an invalid descriptor can still carry
XDP_PKT_CONTD), which is worth calling out for future readers.

>  
>  	struct list_head map_list;
>  	/* Protects map_list */
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a7a83dc4546a..12a845d012f6 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -1063,11 +1077,21 @@ static int __xsk_generic_xmit(struct sock *sk)
>  			goto out;
>  		}
>  
> +		if (unlikely(xs->drain_cont)) {
> +			xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> +			xs->tx->invalid_descs++;
> +			xskq_cons_release(xs->tx);
> +			xs->drain_cont = xp_mb_desc(&desc);
> +			continue;
> +		}
> +
>  		skb = xsk_build_skb(xs, &desc);
>  		if (IS_ERR(skb)) {
>  			err = PTR_ERR(skb);
>  			if (err != -EOVERFLOW)
>  				goto out;
> +			if (xp_mb_desc(&desc))
> +				xs->drain_cont = true;

[Low]
Is the xp_mb_desc(&desc) check here ever false in practice? Looking at
xsk_build_skb() in net/xdp/xsk.c, the only place -EOVERFLOW is returned
is:

    if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc)))
            return ERR_PTR(-EOVERFLOW);

so reaching this point already implies xp_mb_desc(&desc) is true, and
the conditional appears to be a tautology.

Would it read more clearly as an unconditional assignment with a
comment noting the invariant, or alternatively a comment explaining
that a future non-CONTD -EOVERFLOW return would also want draining?
As written, adding a new -EOVERFLOW return site could silently
bypass the drain machinery.

>  			err = 0;
>  			continue;
>  		}
> @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)
>  			goto out;
>  		}
>  
> -		sent_frame = true;
>  		xs->skb = NULL;
>  	}
>  
>  	if (xskq_has_descs(xs->tx)) {
> +		bool drain = xs->skb || xs->drain_cont || xp_mb_desc(&desc);
> +
> +		err = xsk_cq_reserve_locked(xs->pool);
> +		if (err) {
> +			xs->tx->invalid_descs--;

[Low]
Can this decrement of xs->tx->invalid_descs be made less fragile?

The value being cancelled here appears to be the increment performed
inside xskq_cons_is_valid_desc() as called from
xskq_cons_peek_desc(), which incremented invalid_descs before
returning false and dropping out of the while loop. Nothing at the
decrement site documents this cross-function dependency.

invalid_descs is a u64 exposed to userspace through XDP_STATISTICS,
so if the increment location is ever refactored, or if a future path
lets xskq_has_descs() be true here without having gone through a
validity-check failure, this decrement will underflow the counter to
~2^64-1.

Would a short comment naming the paired increment site, or an explicit
save/restore around the peek, make the invariant more robust to future
changes?

> +			if (xs->skb)
> +				xsk_drop_skb(xs->skb);
> +			xs->drain_cont = drain;
> +			err = -EAGAIN;
> +			goto out;
> +		}
> +
>  		if (xs->skb)
>  			xsk_drop_skb(xs->skb);
> +
> +		xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> +
>  		xskq_cons_release(xs->tx);
> +		xs->drain_cont = xp_mb_desc(&desc);
>  	}