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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Oct 26, 2026 at 09:16:07AM +0000, Paul Moses wrote:
> 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>

I skimmed thru clashiko output and nothing major stands out to me. For
your future contributions - multiple patches sent as change set require to
have the cover letter.

For the series:
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

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