Re: [PATCH net v5 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Unconditional rejection of `XDP_UMEM_TX_SW_CSUM` breaks valid native zero-copy configurations.
> - [High] The root cause of the 'wedged TX ring' remains unaddressed, allowing trivial userspace triggers.
> --
>
> commit a1862dee263d88bc3b6ca0898f858a88e2e37fce
> Author: Jason Xing <[email protected]>
>
> xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
>
> This commit attempts to prevent silent per-packet failures and wedged TX
> rings when using sw-csum with drivers advertising IFF_TX_SKB_NO_LINEAR.
> It does so by rejecting the bind operation with -EOPNOTSUPP during setup.
>
> Closes: https://lore.kernel.org/all/[email protected]/#t
> Fixes: 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")
> Acked-by: Stanislav Fomichev <[email protected]>
> Signed-off-by: Jason Xing <[email protected]>
>
> > Without the patch, every descriptor carrying 'XDP_TX_METADATA |
> > XDP_TXMD_FLAGS_CHECKSUM' produces:
> > 1) a WARN_ONCE "offset (N) >= skb_headlen() (0)" from skb_checksum_help(),
> > 2) sendmsg() returning -EINVAL without consuming the descriptor
> >    (invalid_descs is not incremented),
> > 3) a wedged TX ring: __xsk_generic_xmit() does not advance the
> >     consumer on non-EOVERFLOW errors, so the next sendmsg() re-reads
> >     the same descriptor and re-hits the same WARN until the socket
> >     is closed.
>
> Does this completely resolve the wedged TX ring issue? While this patch
> prevents skb_checksum_help() from returning -EINVAL, xsk_skb_metadata()
> can still return -EINVAL for other reasons, such as an out-of-bounds
> csum_start or csum_offset provided by userspace.
>
> If xsk_build_skb() returns -EINVAL, wouldn't __xsk_generic_xmit() still
> evaluate if (err != -EOVERFLOW) goto out; and break its loop without
> consuming the descriptor? Might the generic transmit path need to be
> updated to consume the descriptor and drop the packet on terminal errors
> like -EINVAL to fully prevent userspace from wedging the ring?

I believe we've 'discussed' this before. This is a fresh new and
pre-existing bug (happending) unrelated to this one. A few days ago,
thanks to your reminder, it was put on my to-do list.

>
> > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > index cd7bc50872f6..d981cfdd8535 100644
> > --- a/net/xdp/xsk_buff_pool.c
> > +++ b/net/xdp/xsk_buff_pool.c
> > @@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> >       if (force_zc && force_copy)
> >               return -EINVAL;
> >
> > +     if (pool->tx_sw_csum && (netdev->priv_flags & IFF_TX_SKB_NO_LINEAR))
> > +             return -EOPNOTSUPP;
> > +
>
> Will this check unconditionally reject valid native zero-copy configurations?
>
> In native zero-copy mode (when force_zc is set), the transmission path
> bypasses __xsk_generic_xmit() entirely and processes descriptors natively,
> meaning it does not use xsk_build_skb_zerocopy() or invoke
> skb_checksum_help().
>
> If a UMEM legitimately has the XDP_UMEM_TX_SW_CSUM flag set (for example,
> when shared across multiple devices), it appears this will now return
> -EOPNOTSUPP. Should this check be restricted to only apply when using the
> generic datapath?

The same topic we 'discussed' before. No one is using the software
checksum feature on zerocopy mode. People will be warned because of
wrong usage and then adjust their own codes correspondingly.

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1