[PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[Paul Moses]

Preemptible queues require software padding so hardware includes the
padding bytes in the mCRC. Padding currently runs after TX timestamp
reservation takes an extra skb reference. If padding needs to expand
the skb, pskb_expand_head() rejects the shared skb.

Commit c7d8bc3dd511 ("net: skbuff: don't BUG() on bad arguments to
pskb_expand_head()") replaced the BUG_ON() with -EINVAL, exposing the
padding error path. skb_padto() drops one reference on failure, and
out_drop drops another. This can free the skb while the pending TX
timestamp still holds its pointer.

Move padding before TX timestamp reservation and use __skb_put_padto()
with free_on_error set to false so out_drop handles the skb release.
Preserve the VLAN flags in first->tx_flags when adding timestamp flags.

Fixes: 17643482e9ff ("igc: add preemptible queue support in taprio")
Signed-off-by: Paul Moses <p@1g4.org>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 39 +++++++++++------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 1fb5f3cbe93c..f1bf4e462a51 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1661,6 +1661,23 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		goto out_drop;
 	}
 
+	if (skb_vlan_tag_present(skb)) {
+		tx_flags |= IGC_TX_FLAGS_VLAN;
+		tx_flags |= (skb_vlan_tag_get(skb) << IGC_TX_FLAGS_VLAN_SHIFT);
+	}
+
+	/* record initial flags and protocol */
+	first->tx_flags = tx_flags;
+	first->protocol = protocol;
+
+	/* For preemptible queue, manually pad the skb so that HW includes
+	 * padding bytes in mCRC calculation
+	 */
+	if (tx_ring->preemptible && skb->len < ETH_ZLEN) {
+		if (__skb_put_padto(skb, ETH_ZLEN, false))
+			goto out_drop;
+	}
+
 	if (unlikely(test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags) &&
 		     skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		unsigned long flags;
@@ -1669,10 +1686,10 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 		if (igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-			tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
+			first->tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
 			if (skb->sk &&
 			    READ_ONCE(skb->sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC)
-				tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
+				first->tx_flags |= IGC_TX_FLAGS_TSTAMP_TIMER_1;
 		} else {
 			adapter->tx_hwtstamp_skipped++;
 		}
@@ -1680,24 +1697,6 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
 
-	if (skb_vlan_tag_present(skb)) {
-		tx_flags |= IGC_TX_FLAGS_VLAN;
-		tx_flags |= (skb_vlan_tag_get(skb) << IGC_TX_FLAGS_VLAN_SHIFT);
-	}
-
-	/* record initial flags and protocol */
-	first->tx_flags = tx_flags;
-	first->protocol = protocol;
-
-	/* For preemptible queue, manually pad the skb so that HW includes
-	 * padding bytes in mCRC calculation
-	 */
-	if (tx_ring->preemptible && skb->len < ETH_ZLEN) {
-		if (skb_padto(skb, ETH_ZLEN))
-			goto out_drop;
-		skb_put(skb, ETH_ZLEN - skb->len);
-	}
-
 	tso = igc_tso(tx_ring, first, launch_time, first_flag, &hdr_len);
 	if (tso < 0)
 		goto out_drop;

base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e

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

[Paul Moses]

igc_request_tx_tstamp() takes an extra skb reference before transmit
preparation. For GSO packets, igc_tso() calls skb_cow_head(), which can
reach pskb_expand_head() with a shared skb when the header is cloned.
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.

For non-GSO packets with CHECKSUM_PARTIAL, igc_tx_csum() falls back to
skb_checksum_help() for unsupported checksum offsets. This can also
reach pskb_expand_head() through skb_ensure_writable(). igc_tx_csum()
ignores the error, allowing transmission to continue without completing
the checksum.

Move TSO and checksum preparation before TX timestamp reservation so
the extra skb reference cannot interfere with either operation.

Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Fixes: f38b782dccab ("igc: Add support for TSO")
Signed-off-by: Paul Moses <p@1g4.org>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index f1bf4e462a51..cd4c9815fca7 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);
+
 	if (unlikely(test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags) &&
 		     skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		unsigned long flags;
@@ -1697,12 +1703,6 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
 
-	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);
-
 	igc_tx_map(tx_ring, first, hdr_len);
 
 	return NETDEV_TX_OK;

[PATCH net 3/3] igb: prepare TSO and checksum offloads before TX timestamps

[Paul Moses]

igb_xmit_frame_ring() takes an extra skb reference for TX timestamping
before transmit preparation. For GSO packets, igb_tso() calls
skb_cow_head(), which can reach pskb_expand_head() with a shared skb
when the header is cloned. 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.

For non-GSO packets with CHECKSUM_PARTIAL, igb_tx_csum() falls back to
skb_checksum_help() for unsupported checksum offsets. This can also
reach pskb_expand_head() through skb_ensure_writable(). igb_tx_csum()
ignores the error, allowing transmission to continue without completing
the checksum.

Move TSO and checksum preparation before taking the timestamp reference.
Add the timestamp bit to first->tx_flags without overwriting the offload
flags.

Fixes: 1f6e8178d685 ("igb: Prevent dropped Tx timestamps via work items and interrupts.")
Fixes: 6e033700887b ("igb: Add support for generic Tx checksums")
Signed-off-by: Paul Moses <p@1g4.org>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 31 ++++++++++++-----------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index d4a897a8c82c..04c0053dbaae 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6559,6 +6559,21 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 	first->bytecount = skb->len;
 	first->gso_segs = 1;
 
+	if (skb_vlan_tag_present(skb)) {
+		tx_flags |= IGB_TX_FLAGS_VLAN;
+		tx_flags |= (skb_vlan_tag_get(skb) << IGB_TX_FLAGS_VLAN_SHIFT);
+	}
+
+	/* record initial flags and protocol */
+	first->tx_flags = tx_flags;
+	first->protocol = protocol;
+
+	tso = igb_tso(tx_ring, first, &hdr_len);
+	if (tso < 0)
+		goto out_drop;
+	else if (!tso)
+		igb_tx_csum(tx_ring, first);
+
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
 
@@ -6567,6 +6582,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 					   &adapter->state)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= IGB_TX_FLAGS_TSTAMP;
+			first->tx_flags |= IGB_TX_FLAGS_TSTAMP;
 
 			adapter->ptp_tx_skb = skb_get(skb);
 			adapter->ptp_tx_start = jiffies;
@@ -6577,21 +6593,6 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
 		}
 	}
 
-	if (skb_vlan_tag_present(skb)) {
-		tx_flags |= IGB_TX_FLAGS_VLAN;
-		tx_flags |= (skb_vlan_tag_get(skb) << IGB_TX_FLAGS_VLAN_SHIFT);
-	}
-
-	/* record initial flags and protocol */
-	first->tx_flags = tx_flags;
-	first->protocol = protocol;
-
-	tso = igb_tso(tx_ring, first, &hdr_len);
-	if (tso < 0)
-		goto out_drop;
-	else if (!tso)
-		igb_tx_csum(tx_ring, first);
-
 	if (igb_tx_map(tx_ring, first, hdr_len))
 		goto cleanup_tx_tstamp;
 

Re: [PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[Paul Moses]

On Monday, October 26th, 2026 at 4:20 AM, netdev-bot+sinfo@kernel.org <netdev-bot+sinfo@kernel.org> wrote:

> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.
> 

Originally found during igc development.

[11865.112180] kernel BUG at pskb_expand_head+0x3f3/0x400 [verbose debug info unavailable]
[11865.112206] Oops: invalid opcode: 0000 [#1] SMP NOPTI
[11865.324662] CPU: 7 UID: 0 PID: 91702 Comm: t Not tainted 7.3.0-rc6-nokasan+ #5 PREEMPT
[11865.429202] RIP: 0010:pskb_expand_head+0x3f3/0x400
[11865.486782] Code: 00 48 83 c7 08 e8 dd da ab ff e9 d2 fd ff ff 4c 8b ab c0 00 00 00 44 8b b3 bc 00 00 00 43 0f b6 44 35 00 e9 89 fd ff ff 0f 0b <0f> 0b 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90
[11865.712331] RSP: 0018:ffffaaf3029077e8 EFLAGS: 00010202
[11865.775131] RAX: 0000000000000002 RBX: ffff9e3260c92920 RCX: 0000000000000820
[11865.860902] RDX: 00000000ffffffc6 RSI: 0000000000000000 RDI: ffff9e3260c92920
[11865.946667] RBP: 0000000000000001 R08: 0000000000000003 R09: 0000000000000001
[11866.032435] R10: 0000000000000000 R11: ffffffffc060c8e0 R12: ffff9e3260c92994
[11866.118206] R13: 0000000000000140 R14: 0000000000000005 R15: 000000000000003c
[11866.203979] FS:  00007860e5343e78(0000) GS:ffff9e3770bd5000(0000) knlGS:0000000000000000
[11866.301224] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[11866.370293] CR2: 00007860e531ac90 CR3: 00000005a7e60002 CR4: 0000000000f70ef0
[11866.456051] PKRU: 55555554
[11866.488599] Call Trace:
[11866.518015]  <TASK>
[11866.543266]  __skb_pad+0xbf/0x130
[11866.583116]  igc_xmit_frame+0x428/0xec0 [igc]
[11866.635491]  ? netif_skb_features+0x230/0x330
[11866.687859]  dev_hard_start_xmit+0x9e/0x1b0
[11866.738142]  sch_direct_xmit+0xde/0x260
[11866.784249]  ? __dev_queue_xmit+0x99/0xf20
[11866.833495]  __dev_queue_xmit+0x77d/0xf20
[11866.881698]  ip_finish_output2+0x310/0x390
[11866.930951]  ip_output+0x64/0x100
[11866.970811]  __ip_queue_xmit+0x485/0x4a0
[11867.017973]  ? __skb_clone+0x2a/0x120
[11867.062009]  __tcp_transmit_skb+0xa79/0xc80
[11867.112299]  tcp_write_xmit+0x849/0x1090
[11867.159463]  __tcp_push_pending_frames+0x32/0xc0
[11867.214967]  tcp_sendmsg_locked+0xeb3/0x1110
[11867.266312]  tcp_sendmsg+0x2c/0x50
[11867.307211]  __sock_sendmsg+0x67/0x90
[11867.351251]  __sys_sendto+0x242/0x290
[11867.395277]  __x64_sys_sendto+0x26/0x30
[11867.441393]  do_syscall_64+0xdb/0x310
[11867.485420]  ? do_syscall_64+0x42/0x310
[11867.531533]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[11867.592248] RIP: 0033:0x7860e532f19d
[11867.635231] Code: c0 0f 85 24 00 00 00 49 89 fb 48 89 f0 48 89 d7 48 89 ce 4c 89 c2 4d 89 ca 4c 8b 44 24 08 4c 8b 4c 24 10 4c 89 5c 24 08 0f 05 <c3> e9 1d bb ff ff cc cc cc cc cc cc cc cc cc cc cc cc cc 41 56 53
[11867.860798] RSP: 002b:00007ffdb9a150e8 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
[11867.951782] RAX: ffffffffffffffda RBX: 000000000000002c RCX: 00007860e532f19d
[11868.037552] RDX: 0000000000000001 RSI: 00007ffdb9a1514f RDI: 0000000000000003
[11868.123327] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[11868.209105] R10: 0000000000000000 R11: 0000000000000246 R12: 00007860e5343e78
[11868.294873] R13: 00007ffdb9a151c0 R14: 0000000000000000 R15: 00007860e5343eb4
[11868.380648]  </TASK>

Tested on I210 and I225-V

Re: [PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[Eric Dumazet]

Le lun. 26 oct. 2026 à 10:16, Paul Moses <p@1g4.org> a écrit :
>
> Preemptible queues require software padding so hardware includes the
> padding bytes in the mCRC. Padding currently runs after TX timestamp
> reservation takes an extra skb reference. If padding needs to expand
> the skb, pskb_expand_head() rejects the shared skb.
>
> Commit c7d8bc3dd511 ("net: skbuff: don't BUG() on bad arguments to
> pskb_expand_head()") replaced the BUG_ON() with -EINVAL, exposing the
> padding error path. skb_padto() drops one reference on failure, and
> out_drop drops another. This can free the skb while the pending TX
> timestamp still holds its pointer.
>
> Move padding before TX timestamp reservation and use __skb_put_padto()
> with free_on_error set to false so out_drop handles the skb release.
> Preserve the VLAN flags in first->tx_flags when adding timestamp flags.
>
> Fixes: 17643482e9ff ("igc: add preemptible queue support in taprio")
> Signed-off-by: Paul Moses <p@1g4.org>

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

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

[Eric Dumazet]

Le lun. 26 oct. 2026 à 10:16, Paul Moses <p@1g4.org> a écrit :
>
> igc_request_tx_tstamp() takes an extra skb reference before transmit
> preparation. For GSO packets, igc_tso() calls skb_cow_head(), which can
> reach pskb_expand_head() with a shared skb when the header is cloned.
> 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.
>
> For non-GSO packets with CHECKSUM_PARTIAL, igc_tx_csum() falls back to
> skb_checksum_help() for unsupported checksum offsets. This can also
> reach pskb_expand_head() through skb_ensure_writable(). igc_tx_csum()
> ignores the error, allowing transmission to continue without completing
> the checksum.
>
> Move TSO and checksum preparation before TX timestamp reservation so
> the extra skb reference cannot interfere with either operation.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Fixes: f38b782dccab ("igc: Add support for TSO")
> Signed-off-by: Paul Moses <p@1g4.org>
> ---

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

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

[Eric Dumazet]

Le lun. 26 oct. 2026 à 10:16, Paul Moses <p@1g4.org> a écrit :
>
> igb_xmit_frame_ring() takes an extra skb reference for TX timestamping
> before transmit preparation. For GSO packets, igb_tso() calls
> skb_cow_head(), which can reach pskb_expand_head() with a shared skb
> when the header is cloned. 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.
>
> For non-GSO packets with CHECKSUM_PARTIAL, igb_tx_csum() falls back to
> skb_checksum_help() for unsupported checksum offsets. This can also
> reach pskb_expand_head() through skb_ensure_writable(). igb_tx_csum()
> ignores the error, allowing transmission to continue without completing
> the checksum.
>
> Move TSO and checksum preparation before taking the timestamp reference.
> Add the timestamp bit to first->tx_flags without overwriting the offload
> flags.
>
> Fixes: 1f6e8178d685 ("igb: Prevent dropped Tx timestamps via work items and interrupts.")
> Fixes: 6e033700887b ("igb: Add support for generic Tx checksums")
> Signed-off-by: Paul Moses <p@1g4.org>

Reviewed-by: Eric Dumazet <edumazet@kernel.org>

Re: [PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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

- [High] After this patch alone, igc_xmit_frame_ring() still calls
  igc_request_tx_tstamp(), which does skb_get() (line ~1687), before…
- [Low] The commit message describes a failure that cannot happen on the
  tree this patch applies to.

--

igc: pad preemptible packets before reserving TX timestamps

This commit moves the preemptible-queue padding in igc_xmit_frame_ring()
so it runs before the TX timestamp reservation. The extra reference taken
by igc_request_tx_tstamp() is then not yet held when padding may call
pskb_expand_head(). It also switches to __skb_put_padto() with
free_on_error set to false, so out_drop is the only place that frees the
skb.

> Commit c7d8bc3dd511 ("net: skbuff: don't BUG() on bad arguments to
> pskb_expand_head()") replaced the BUG_ON() with -EINVAL, exposing the
> padding error path. skb_padto() drops one reference on failure, and
> out_drop drops another. This can free the skb while the pending TX
> timestamp still holds its pointer.

[Severity: Low]
Does this description match the tree this patch applies to? Here,
pskb_expand_head() in net/core/skbuff.c still has:

	BUG_ON(nhead < 0);

	BUG_ON(skb_shared(skb));

So c7d8bc3dd511 does not appear to be applied. Without it, take a short
frame on a preemptible queue that is cloned or lacks tailroom. Before
this patch it would go through:

igc_xmit_frame_ring()
  igc_request_tx_tstamp()
    tstamp->skb = skb_get(skb);    /* skb now shared */
  skb_padto()
    __skb_pad()
      pskb_expand_head()
        BUG_ON(skb_shared(skb));

That gives a BUG() rather than the -EINVAL and use-after-free described
above. The same holds for stable trees that carry 17643482e9ff but not
c7d8bc3dd511.

The code change covers both cases, since padding now runs before
skb_get(). Could the commit message say that kernels without
c7d8bc3dd511 crash in pskb_expand_head() instead?

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c5..f1bf4e462a511 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c

[ ... ]

> @@ -1669,10 +1686,10 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  		if (igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
>  			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;

[ ... ]

> @@ -1680,24 +1697,6 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  	}
>  

[ ... ]

> -	if (tx_ring->preemptible && skb->len < ETH_ZLEN) {
> -		if (skb_padto(skb, ETH_ZLEN))
> -			goto out_drop;
> -		skb_put(skb, ETH_ZLEN - skb->len);
> -	}
> -
>  	tso = igc_tso(tx_ring, first, launch_time, first_flag, &hdr_len);
>  	if (tso < 0)
>  		goto out_drop;

[Severity: High]
With this patch applied on its own, igc_request_tx_tstamp() still runs
before igc_tso() and igc_tx_csum(). Can the same shared skb problem come
up here?

When a GSO skb with a cloned header (for example with a packet tap or tc
mirred active) is hardware timestamped:

igc_xmit_frame_ring()
  igc_request_tx_tstamp()
    tstamp->skb = skb_get(skb);
  igc_tso()
    skb_cow_head(skb, 0)
      __skb_cow()
        pskb_expand_head()
          BUG_ON(skb_shared(skb));

igc_tx_csum() can reach pskb_expand_head() the same way, through
skb_checksum_help()->skb_ensure_writable(), on a cloned skb.

The next patch in this series, "igc: prepare TSO and checksum offloads
before TX timestamps", moves igc_tso() and igc_tx_csum() ahead of the
timestamp reservation. That fixes this by the end of the series. Should
the two patches be backported together, since this one alone leaves the
TSO and checksum paths exposed?

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

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

[netdev-bot+sashiko]

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

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

[netdev-bot+sashiko]

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

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

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] igb_tx_csum() still ignores the return value of
  skb_checksum_help() (igb_main.c:6170, `default:…
- [Low] The commit message says the bug's effect is a dropped packet (TSO
  path) or an incomplete checksum (csum path), because commit…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  ixgbe_xmit_frame_ring() has the same ordering bug this series fixes in…
- [Medium] This problem wasn't introduced by this patch, but on the
  igb_tx_map() failure path, cleanup_tx_tstamp in igb_xmit_frame_ring()…
- [Medium] This problem wasn't introduced by this patch, but the
  cleanup_tx_tstamp path in igb_xmit_frame_ring() calls…

--

igb: prepare TSO and checksum offloads before TX timestamps

This patch moves the VLAN flag setup, igb_tso() and igb_tx_csum() in
igb_xmit_frame_ring() ahead of the skb_get() taken for hardware TX
timestamping. Offload preparation therefore no longer runs on a shared skb.
The timestamp flag is then ORed into first->tx_flags so the offload flags
are kept.

> igb_xmit_frame_ring() takes an extra skb reference for TX timestamping
> before transmit preparation. For GSO packets, igb_tso() calls
> skb_cow_head(), which can reach pskb_expand_head() with a shared skb
> when the header is cloned. 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: Low]
Does this description match the tree this patch applies to?

Commit c7d8bc3dd511 doesn't appear to be an ancestor of this patch.
pskb_expand_head() here still has:

net/core/skbuff.c:pskb_expand_head() {
    ...
	BUG_ON(skb_shared(skb));
    ...
}

The same is true for the stable kernels the Fixes: tags point at.

So before this patch, both paths described in the message would hit BUG().
They would not drop the packet or skip the checksum:

igb_xmit_frame_ring() /* skb_get() */
  igb_tso()->skb_cow_head()->__skb_cow()->pskb_expand_head()

igb_xmit_frame_ring() /* skb_get() */
  igb_tx_csum()->skb_checksum_help()->skb_ensure_writable()
    ->pskb_expand_head()

The code change looks right. Could the message say that the effect is a
BUG()? Once hardware TX timestamping is enabled, an SO_TIMESTAMPING sender
can trigger it.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index d4a897a8c82c6..04c0053dbaae8 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6559,6 +6559,21 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
>  	first->bytecount = skb->len;
>  	first->gso_segs = 1;
>  
> +	if (skb_vlan_tag_present(skb)) {
> +		tx_flags |= IGB_TX_FLAGS_VLAN;
> +		tx_flags |= (skb_vlan_tag_get(skb) << IGB_TX_FLAGS_VLAN_SHIFT);
> +	}
> +
> +	/* record initial flags and protocol */
> +	first->tx_flags = tx_flags;
> +	first->protocol = protocol;
> +
> +	tso = igb_tso(tx_ring, first, &hdr_len);
> +	if (tso < 0)
> +		goto out_drop;
> +	else if (!tso)
> +		igb_tx_csum(tx_ring, first);

[Severity: Medium]
The commit message says igb_tx_csum() ignores the error from
skb_checksum_help(). This patch removes the shared skb trigger, but can
that ignored error still happen?

igb_tx_csum() still does:

	default:
		skb_checksum_help(skb);
		goto csum_failed;

igb advertises NETIF_F_HW_CSUM. A non-GSO CHECKSUM_PARTIAL skb with an
unusual csum_offset therefore ends up here, for example one sent through
AF_PACKET or tun with a vnet_hdr.

skb_checksum_help() can still return early without writing the checksum or
setting CHECKSUM_NONE:

skb_checksum_help()
  skb_ensure_writable()
    pskb_expand_head(skb, 0, 0, GFP_ATOMIC)  /* -ENOMEM, cloned header */

The other early returns are a __skb_linearize() failure, the -EINVAL
headlen offset checks, and !skb_frags_readable().

On failure igb_tx_csum() jumps to csum_failed. Because igb_tx_csum() returns
void, igb_xmit_frame_ring() cannot drop the packet. igb_tx_map() then sends
a frame whose L4 checksum field holds only the pseudo-header partial sum.

Could igb_tx_csum() return an error so the caller can goto out_drop? That
would match how an igb_tso() failure is handled here, and what
validate_xmit_skb() does when skb_checksum_help() fails.

> +
>  	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>  		struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
>  
> @@ -6567,6 +6582,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
>  					   &adapter->state)) {
>  			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  			tx_flags |= IGB_TX_FLAGS_TSTAMP;
> +			first->tx_flags |= IGB_TX_FLAGS_TSTAMP;
>  
>  			adapter->ptp_tx_skb = skb_get(skb);
>  			adapter->ptp_tx_start = jiffies;

[Severity: High]
This isn't a bug introduced by this patch, but does ixgbe_xmit_frame_ring()
have the same ordering problem that this series fixes in igb and igc?

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

After skb_get(), three paths can call pskb_expand_head() on the now shared
skb when its header is cloned:

  the DCB software VLAN skb_cow_head()
  ixgbe_tso()->skb_cow_head()->__skb_cow()
  ixgbe_tx_csum()->skb_checksum_help()->skb_ensure_writable()

In this tree that hits BUG_ON(skb_shared(skb)) in pskb_expand_head(). Once
c7d8bc3dd511 is applied, it becomes a dropped TSO packet or an ignored
checksum failure instead.

The series doesn't touch ixgbe. Should ixgbe get the same reordering?

[ ... ]

> @@ -6577,21 +6593,6 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,

[ ... ]

>  	if (igb_tx_map(tx_ring, first, hdr_len))
>  		goto cleanup_tx_tstamp;

[Severity: Medium]
This is a pre-existing issue, but on 82576 can the cleanup_tx_tstamp path
race with igb_ptp_tx_work()?

igb_xmit_frame_ring() {
    ...
cleanup_tx_tstamp:
	if (unlikely(tx_flags & IGB_TX_FLAGS_TSTAMP)) {
		...
		dev_kfree_skb_any(adapter->ptp_tx_skb);
		adapter->ptp_tx_skb = NULL;
		if (adapter->hw.mac.type == e1000_82576)
			cancel_work_sync(&adapter->ptp_tx_work);
    ...
}

The skb is freed before the work is synchronized. igb_ptp_tx_work() checks
and uses the same pointer without a lock:

igb_ptp_tx_work() {
	if (!adapter->ptp_tx_skb)
		return;

	if (time_is_before_jiffies(adapter->ptp_tx_start +
				   IGB_PTP_TX_TIMEOUT)) {
		dev_kfree_skb_any(adapter->ptp_tx_skb);
    ...
	tsynctxctl = rd32(E1000_TSYNCTXCTL);
	if (tsynctxctl & E1000_TSYNCTXCTL_VALID)
		igb_ptp_tx_hwtstamp(adapter);
    ...
}

igb_ptp_tx_hwtstamp() copies adapter->ptp_tx_skb and later calls
skb_tstamp_tx() and dev_kfree_skb_any() on that copy.

The work can be running on another CPU, either scheduled by
igb_tsync_interrupt() on E1000_TSICR_TXTS or rescheduled by itself. If it
passes the NULL check before the xmit CPU frees the skb, could it use or
free an skb that has already been freed?

To hit this, the worker would need a stale TSYNCTXCTL_VALID bit or a stale
ptp_tx_start, so it may be hard to trigger. The code comes from commit
74344e32fcc0. This patch makes it harder to reach, because igb_tso()
failures now return before IGB_TX_FLAGS_TSTAMP is set.

[Severity: Medium]
This is also pre-existing and not introduced by this patch, but is
cancel_work_sync() safe to call in this path?

igb_xmit_frame_ring() runs as ndo_start_xmit. BH is disabled under
rcu_read_lock_bh(), and the txq _xmit_lock is held. ptp_tx_work is a regular
work item, so __cancel_work_sync() calls might_sleep(). It can also block in
__flush_work() if the work is running.

With CONFIG_DEBUG_ATOMIC_SLEEP, each igb_tx_map() DMA mapping failure on a
timestamped 82576 packet would print a "sleeping function called from
invalid context" splat.

If the work is running at that moment, the xmit path would really sleep in
atomic context. That can happen because igb_ptp_tx_hwtstamp() clears
__IGB_PTP_TX_IN_PROGRESS before it calls skb_tstamp_tx(). A new timestamp
request can then start while the previous worker is still running.

After this patch, only an igb_tx_map() failure reaches this path. This code
also comes from commit 74344e32fcc0.

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

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

[Paul Moses]

> [Severity: Low]
> Does this description match the tree this patch applies to?

It needs to be clear that it is affected by that commit.

> - [Medium] igb_tx_csum() still ignores the return value of
>   skb_checksum_help() (igb_main.c:6170, `default:…

out of scope

> - [High] This problem wasn't introduced by this patch, but
>   ixgbe_xmit_frame_ring() has the same ordering bug this series fixes in…

fixed

> - [Medium] This problem wasn't introduced by this patch, but on the
>   igb_tx_map() failure path, cleanup_tx_tstamp in igb_xmit_frame_ring()…
> - [Medium] This problem wasn't introduced by this patch, but the
>   cleanup_tx_tstamp path in igb_xmit_frame_ring() calls…

out of scope

No plans for immediate followups, anyone/everyone feel free.

Re: [PATCH net 1/3] igc: pad preemptible packets before reserving TX timestamps

[Paul Moses]

> [Severity: Low]
> Does this description match the tree this patch applies to?

It needs to be clear that it is affected by that commit.

> [Severity: High]
> With this patch applied on its own, igc_request_tx_tstamp() still runs
> before igc_tso() and igc_tx_csum(). Can the same shared skb problem come
> up here?
> 
> When a GSO skb with a cloned header (for example with a packet tap or tc
> mirred active) is hardware timestamped:
> 
> igc_xmit_frame_ring()
>   igc_request_tx_tstamp()
>     tstamp->skb = skb_get(skb);
>   igc_tso()
>     skb_cow_head(skb, 0)
>       __skb_cow()
>         pskb_expand_head()
>           BUG_ON(skb_shared(skb));
> 
> igc_tx_csum() can reach pskb_expand_head() the same way, through
> skb_checksum_help()->skb_ensure_writable(), on a cloned skb.
> 
> The next patch in this series, "igc: prepare TSO and checksum offloads
> before TX timestamps", moves igc_tso() and igc_tx_csum() ahead of the
> timestamp reservation. That fixes this by the end of the series. Should
> the two patches be backported together, since this one alone leaves the
> TSO and checksum paths exposed?
> 

Yes, I maybe need to research how to properly denote this