Re: [PATCH RFC net-next v4 07/14] xsk: try to skip validating skb list in xmit path

On Wed, Apr 15, 2026 at 5:33 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [Critical] Bypassing `validate_xmit_skb_list()` based on `NETIF_F_SG` skips critical hardware capability checks, software fallbacks, and SKB initialization, leading to kernel panics and corrupted packets.
> - [High] Breaking out of the transmit loop early leaves packets stuck in the TX ring indefinitely because the hardware doorbell is never flushed.
> --
>
> commit 7603ac608a36b8d39e026a60b567ea2b106617f0
> Author: Jason Xing <[email protected]>
>
> xsk: try to skip validating skb list in xmit path
>
> This patch attempts to optimize the XSK transmit path by skipping
> validate_xmit_skb_list() if the device supports Scatter-Gather,
> aiming to improve performance for drivers like ixgbe.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 99188353abec6..12643cbab6530 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> [ ... ]
> > @@ -4878,15 +4879,17 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
> >       local_bh_disable();
> >       HARD_TX_LOCK(dev, txq, smp_processor_id());
> >       while ((skb = __skb_dequeue(send_queue)) != NULL) {
> > -             struct sk_buff *orig_skb = skb;
> > -             bool again = false;
> > -
> > -             skb = validate_xmit_skb_list(skb, dev, &again);
> > -             if (skb != orig_skb) {
> > -                     dev_core_stats_tx_dropped_inc(dev);
> > -                     kfree_skb_list(skb);
> > -                     ret = NET_XMIT_DROP;
> > -                     break;
> > +             if (unlikely(need_validate)) {
> > +                     struct sk_buff *orig_skb = skb;
> > +                     bool again = false;
> > +
> > +                     skb = validate_xmit_skb_list(skb, dev, &again);
>
> Does skipping validate_xmit_skb_list() based on NETIF_F_SG bypass necessary
> hardware capability checks and software fallbacks?
>
> validate_xmit_skb() performs dynamic feature verification via
> ndo_features_check(), transport header initialization via
> skb_set_transport_header(), and software checksum fallbacks via
> skb_csum_hwoffload_help().
>
> AF_XDP users can request TX checksum offload, setting skb->ip_summed to
> CHECKSUM_PARTIAL. By skipping validation solely because the device supports
> Scatter-Gather, packets bypass skb_set_transport_header(), leaving
> skb->transport_header uninitialized.
>
> Could this cause drivers that use skb_transport_offset() to compute massive
> sizes, corrupting TX descriptors and potentially hanging the NIC?
>
> Additionally, if the device lacks NETIF_F_HW_CSUM, skipping
> skb_csum_hwoffload_help() sends CHECKSUM_PARTIAL to drivers unable to handle
> it. Might this result in silent data corruption on the wire or driver crashes?

Makes sense. I will exclude this metadata case (using csum) by adding
a new condition:
if (unlikely(need_validate || skb->ip_summed == CHECKSUM_PARTIAL)) {

>
> > +                     if (skb != orig_skb) {
> > +                             dev_core_stats_tx_dropped_inc(dev);
> > +                             kfree_skb_list(skb);
> > +                             ret = NET_XMIT_DROP;
> > +                             break;
>
> If this loop breaks early, does it leave packets stuck in the TX ring?
>
> While this behavior was introduced earlier in the series and not by this
> specific patch, the issue is present in this modified block.
>
> The batching logic calls netdev_start_xmit() with more = true for all packets
> except the last one in the queue.
>
> If the loop breaks early here due to a validate_xmit_skb_list() failure,
> the function returns without ever calling netdev_start_xmit() with
> more = false.
>
> Will this cause the driver to defer ringing the hardware TX doorbell,
> leaving previously submitted packets stranded in the TX ring and causing
> latency spikes or TX watchdog timeouts?

I answered in patch 6. No need to take action in this series unless
maintainers request me to do this first.

Thanks,
Jason

>
> > +                     }
> >               }
> >
> >               if (netif_xmit_frozen_or_drv_stopped(txq)) {
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7