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