Re: [PATCH net 2/3] igc: prepare TSO and checksum offloads before TX timestamps

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message misstates what the patch fixes.
- [Medium] igc_tx_csum() (drivers/net/ethernet/intel/igc/igc_main.c:1200)
  still ignores the return value of skb_checksum_help().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but ixgbe has the
  same skb_get()-before-skb_cow_head() ordering and the series leaves it…

--

igc: prepare TSO and checksum offloads before TX timestamps

In igc_xmit_frame_ring(), this patch moves the igc_tso() and igc_tx_csum()
calls ahead of the TX hardware timestamp reservation. As a result, the
extra skb reference that igc_request_tx_tstamp() takes no longer exists
while the headers are being prepared.

> Commit c7d8bc3dd511 ("net: skbuff: don't BUG() on bad arguments to
> pskb_expand_head()") replaced the BUG_ON() with -EINVAL, causing TSO
> preparation to fail and the packet to be dropped.

[Severity: Medium]
Is this description accurate for the tree this patch applies to?
c7d8bc3dd511 does not appear to be an ancestor of this commit, and
pskb_expand_head() in net/core/skbuff.c still has:

	BUG_ON(skb_shared(skb));

Before this patch, igc_request_tx_tstamp() does tstamp->skb = skb_get(skb),
which leaves skb->users at 2. If the header is cloned (for example an
AF_PACKET tap clone from dev_queue_xmit_nit(), or a bridge or mirred
clone), these paths are taken:

  igc_tso()->skb_cow_head()->__skb_cow()->pskb_expand_head()
  igc_tx_csum()->skb_checksum_help()->skb_ensure_writable()->pskb_expand_head()

Both would hit that BUG_ON() instead of returning -EINVAL.

The stable trees that the Fixes: tags (2c344ae24501, f38b782dccab) point
at would behave the same way. Could the commit message say that in those
kernels the symptom is a BUG(), not a dropped packet or a missing checksum?

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index f1bf4e462a511..cd4c9815fca7c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1678,6 +1678,12 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  			goto out_drop;
>  	}
>  
> +	tso = igc_tso(tx_ring, first, launch_time, first_flag, &hdr_len);
> +	if (tso < 0)
> +		goto out_drop;
> +	else if (!tso)
> +		igc_tx_csum(tx_ring, first, launch_time, first_flag);

[Severity: Medium]
The commit message says "igc_tx_csum() ignores the error, allowing
transmission to continue without completing the checksum". After this
patch, igc_tx_csum() still throws away the return value:

drivers/net/ethernet/intel/igc/igc_main.c:igc_tx_csum() {
    ...
	default:
		skb_checksum_help(skb);
		goto csum_failed;
    ...
}

The reorder removes the shared skb trigger, but skb_checksum_help() can
still fail in other ways:

- -ENOMEM from __skb_linearize() for shared frags
- -ENOMEM from skb_ensure_writable()->pskb_expand_head(GFP_ATOMIC) when
  the header is cloned
- -EINVAL or -EFAULT for bad offsets or unreadable frags

In each case ip_summed is left as CHECKSUM_PARTIAL with no checksum
written, and igc_xmit_frame_ring() continues to igc_tx_map(). Does the
frame then go out with only the pseudo-header seed in the L4 checksum
field?

The default branch can be reached because igc_probe() advertises
NETIF_F_HW_CSUM. That lets CHECKSUM_PARTIAL skbs with any csum_offset
arrive, for example packets forwarded from tun/tap vnet_hdr guests.

Could igc_tx_csum() return an error and goto out_drop on failure, the
same way the igc_tso() < 0 case is handled?

> +
>  	if (unlikely(test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags) &&
>  		     skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>  		unsigned long flags;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
ixgbe seems to have the same ordering, and this series does not touch it.
Later in the series, igb is fixed by "igb: prepare TSO and checksum
offloads before TX timestamps". ixgbe_xmit_frame_ring() still takes the
timestamp reference before any header preparation:

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_xmit_frame_ring() {
    ...
			adapter->ptp_tx_skb = skb_get(skb);
    ...
			if (skb_cow_head(skb, 0))
				goto out_drop;
    ...
	tso = ixgbe_tso(tx_ring, first, &hdr_len, &ipsec_tx);
    ...
	else if (!tso)
		ixgbe_tx_csum(tx_ring, first, &ipsec_tx);
    ...
}

ixgbe_tso() also calls skb_cow_head(skb, 0) itself. Once HWTSTAMP_TX_ON is
enabled and a socket asks for SOF_TIMESTAMPING_TX_HARDWARE on a GSO skb
with a cloned header, this path runs while skb->users is 2:

  ixgbe_tso()->skb_cow_head()->__skb_cow()->pskb_expand_head()

Wouldn't that hit BUG_ON(skb_shared(skb)) in pskb_expand_head()? The DCB
software VLAN skb_cow_head() call and ixgbe_tx_csum() also run after the
reference is taken.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026091549.605810-1-p%401g4.org