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

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/ (local)

[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.

[ ... ]

[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.

[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);
 	}