Re: [PATCH net 3/7] xsk: drain continuation descs on invalid descriptor in __xsk_generic_xmit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The `invalid_descs` statistic is repeatedly incremented for the same invalid descriptor if the Completion Queue (CQ) is full.

Pre-existing issues:
- [High] A malformed descriptor (e.g., invalid TX metadata) permanently stalls the TX ring because it is never consumed.
--

commit b076f5f0d28ebb521e9e57462363bc0f2250e923
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: drain continuation descs on invalid descriptor in __xsk_generic_xmit()

When the TX loop in __xsk_generic_xmit() encounters an invalid descriptor
mid-packet, the partial skb is dropped and the offending descriptor is
released. This commit ensures remaining continuation descriptors belonging
to the same multi-buffer packet are correctly drained on the next iteration
instead of being treated as a brand-new packet.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index e80c035a7af54..c489fadc36087 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[Severity: High]
This is a pre-existing issue, but does encountering a non-transient error
during skb building permanently stall the TX ring?

In __xsk_generic_xmit(), if xsk_build_skb() returns a non-transient error
like -EINVAL (for example, from an invalid XDP_TX_METADATA payload), it
intentionally skips releasing the descriptor:

net/xdp/xsk.c:xsk_build_skb() {
    ...
    } else {
        /* Let application retry */
        xsk_cq_cancel_locked(xs->pool, 1);
    }
}

Since the error is not -EOVERFLOW, __xsk_generic_xmit() jumps to out
without releasing the descriptor:

net/xdp/xsk.c:__xsk_generic_xmit() {
    ...
    skb = xsk_build_skb(xs, &desc);
    if (IS_ERR(skb)) {
        err = PTR_ERR(skb);
        if (err != -EOVERFLOW)
            goto out;
    ...
}

Because the descriptor remains at the head of the TX ring and the error
is non-transient, will every subsequent call peek the same descriptor,
fail with -EINVAL, and exit without ever advancing?

[ ... ]

> @@ -1128,6 +1128,7 @@ static int __xsk_generic_xmit(struct sock *sk)
>  		if (xs->skb)
>  			xsk_drop_skb(xs->skb);
>  		xskq_cons_release(xs->tx);
> +		xs->drain_cont = xp_mb_desc(&desc);
>  	}
>  
>  out:

[Severity: Medium]
If the completion queue is full, does this post-loop cleanup jump to out
without consuming the invalid descriptor from the TX ring?

Looking earlier in this same xskq_has_descs() block in __xsk_generic_xmit(),
if reclaim_desc is true, it attempts to reserve a CQ slot:

net/xdp/xsk.c:__xsk_generic_xmit() {
    ...
    if (xskq_has_descs(xs->tx)) {
        bool reclaim_desc = xs->skb || xs->drain_cont;
        
        if (reclaim_desc) {
            err = xsk_cq_reserve_locked(xs->pool);
            if (err) {
                err = -EAGAIN;
                goto out;
            }
        }
    ...
}

If xsk_cq_reserve_locked() returns -EAGAIN, we jump to out before reaching
the xskq_cons_release() call in the diff above.

Would the next invocation (via poll or sendmsg) then peek the exact same
unconsumed descriptor?

Since xskq_cons_is_valid_desc() increments invalid_descs every time a
malformed descriptor is peeked:

net/xdp/xsk_queue.h:xskq_cons_is_valid_desc() {
    ...
    if (!xp_validate_desc(pool, d)) {
        q->invalid_descs++;
        return false;
    }
    ...
}

Could this arbitrarily inflate the invalid_descs statistic while the
CQ remains full?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623133240.1048434-1-maciej.fijalkowski@intel.com?part=3