Re: [PATCH] cxgb4: Fix ptp_tx_skb reference leak in cxgb4_eth_xmit()

[netdev-bot+sashiko]

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

Critical: 1 · 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).

- [Critical] cxgb4_eth_xmit() releases the PTP reference based only on
  the pointer identity `adap->ptp_tx_skb == skb`, without knowing…
- [High] Holding adap->ptp_lock in cxgb4_eth_xmit() gives the new
  dev_kfree_skb_any(adap->ptp_tx_skb) calls no exclusion against the…
- [Low] The PTP arming step sets two pieces of state together at
  sge.c:1528-1529 - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS and…

--

cxgb4: Fix ptp_tx_skb reference leak in cxgb4_eth_xmit()

The patch moves the netdev_priv()/pi->adapter lookup to the top of
cxgb4_eth_xmit() and then releases adap->ptp_tx_skb on the NETDEV_TX_BUSY
path and on the out_free path.  Both new blocks decide whether to drop the
extra reference by comparing adap->ptp_tx_skb against skb.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
> index 9fccb8ea9bcd1..5d4c63d9a937a 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c

[ ... ]

> @@ -1553,6 +1554,10 @@ static netdev_tx_t cxgb4_eth_xmit(struct sk_buff *skb, struct net_device *dev)
>  		dev_err(adap->pdev_dev,
>  			"%s: Tx ring %u full while queue awake!\n",
>  			dev->name, qidx);
> +		if (adap->ptp_tx_skb == skb) {
> +			dev_kfree_skb_any(adap->ptp_tx_skb);
> +			adap->ptp_tx_skb = NULL;
> +		}
>  		return NETDEV_TX_BUSY;
>  	}

[Severity: Critical]
Can this release a reference that this invocation no longer owns?

The reference is taken in exactly one place in cxgb4_eth_xmit():

	if (ptp_enabled) {
		if (!(adap->ptp_tx_skb)) {
			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
			adap->ptp_tx_skb = skb_get(skb);
		} else {
			goto out_free;
		}

but the new cleanup infers ownership from the pointer comparison
adap->ptp_tx_skb == skb.  Note the else branch also reaches out_free
without owning a reference.

The completion side drops the reference before it clears the field:

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c:cxgb4_ptp_read_hwstamp() {
	...
	skb_tstamp_tx(adapter->ptp_tx_skb, skb_ts);
	dev_kfree_skb_any(adapter->ptp_tx_skb);
	spin_lock(&adapter->ptp_lock);
	adapter->ptp_tx_skb = NULL;
	spin_unlock(&adapter->ptp_lock);
}

t4_start_xmit() holds ptp_lock across the whole of cxgb4_eth_xmit() for PTP
frames, so the receive-side thread can sit blocked on that spin_lock() with
the reference already dropped while adap->ptp_tx_skb still equals skb:

CPU0, t4_ethrx_handler() -> t4_tx_hststamp() -> cxgb4_ptp_read_hwstamp()
	dev_kfree_skb_any(adapter->ptp_tx_skb);	/* refcount 2 -> 1 */
	spin_lock(&adapter->ptp_lock);		/* blocks, CPU1 holds it */

CPU1, t4_start_xmit() -> spin_lock(&adap->ptp_lock) -> cxgb4_eth_xmit()
	if (adap->ptp_tx_skb == skb) {		/* still true */
		dev_kfree_skb_any(adap->ptp_tx_skb);	/* 1 -> 0, freed */

Here the skb is then handed back to the qdisc with NETDEV_TX_BUSY and
retransmitted, so can this become a use-after-free?  On out_free the
following dev_kfree_skb_any(skb) would free it a second time.

The window is the whole xmit call, not a few instructions, and the
completion path is entered for any inbound UDP frame with source and
destination port 319 (cxgb4_ptp_is_ptp_rx()) without checking that it is
the loopback of the pending skb.  The ptptxq is allocated with 8
descriptors, so credits < 0 is readily reachable.

Would recording ownership locally, for example a bool set where skb_get()
succeeds, and dropping the reference only under that condition and under
ptp_lock, be a safer form of this fix?

Separately, t4_start_xmit() takes ptp_lock only for PTP frames:

	if (is_ptp_enabled(skb, dev)) {
		struct adapter *adap = netdev2adap(dev);
		netdev_tx_t ret;

		spin_lock(&adap->ptp_lock);
		ret = cxgb4_eth_xmit(skb, dev);
		spin_unlock(&adap->ptp_lock);
		return ret;
	}

	return cxgb4_eth_xmit(skb, dev);

so for ordinary traffic the new read and store of adap->ptp_tx_skb run with
no lock at all, against a field documented in struct adapter as covered by
ptp_lock.  Should these accesses be gated on ptp_enabled?

[Severity: Low]
The arming step sets two pieces of state together:

			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
			adap->ptp_tx_skb = skb_get(skb);

Should this abort path also clear SKBTX_IN_PROGRESS?  The skb goes back to
the qdisc with the flag still set, while no work request with
TXPKT_TSTAMP_F was written to adap->sge.ptptxq and adap->ptp_tx_skb has
just been cleared, so t4_tx_hststamp()/cxgb4_ptp_read_hwstamp() can no
longer produce a completion for that frame.

If userspace disables transmit timestamping between the abort and the retry
(cxgb_hwtstamp_set() clearing pi->ptp_enable), the retry takes the ordinary
path, and the stale flag then also suppresses the software timestamp:

net/core/skbuff.c:__skb_tstamp_tx() {
	...
	if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
	    skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
		return;
	...
}

so the SO_TIMESTAMPING consumer gets neither a hardware nor a software
timestamp for that frame.

> @@ -1723,6 +1728,10 @@ static netdev_tx_t cxgb4_eth_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return NETDEV_TX_OK;
>  
>  out_free:
> +	if (adap->ptp_tx_skb == skb) {
> +		dev_kfree_skb_any(adap->ptp_tx_skb);
> +		adap->ptp_tx_skb = NULL;
> +	}
>  	dev_kfree_skb_any(skb);
>  	return NETDEV_TX_OK;
>  }

[Severity: High]
Does holding ptp_lock in cxgb4_eth_xmit() actually exclude the racing user
of this field?  cxgb4_ptp_read_hwstamp() dereferences the skb, reports the
timestamp and drops the reference entirely outside ptp_lock, taking the
lock only to store NULL:

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c:cxgb4_ptp_read_hwstamp() {
	skb_ts = skb_hwtstamps(adapter->ptp_tx_skb);
	...
	skb_tstamp_tx(adapter->ptp_tx_skb, skb_ts);
	dev_kfree_skb_any(adapter->ptp_tx_skb);
	spin_lock(&adapter->ptp_lock);
	adapter->ptp_tx_skb = NULL;
	...
}

and the non-NULL test is done with no lock either:

drivers/net/ethernet/chelsio/cxgb4/sge.c:t4_tx_hststamp() {
	if (!is_t4(adapter->params.chip) && adapter->ptp_tx_skb) {
		cxgb4_ptp_read_hwstamp(adapter, pi);
	...
}

Before this patch the transmit path never stored NULL into
adap->ptp_tx_skb, so this hunk and the NETDEV_TX_BUSY hunk add a second
writer.  Can that produce a NULL dereference?

CPU0, t4_tx_hststamp()
	if (... && adapter->ptp_tx_skb)		/* non-NULL */

CPU1, cxgb4_eth_xmit() out_free
	adap->ptp_tx_skb = NULL;

CPU0, cxgb4_ptp_read_hwstamp()
	skb_ts = skb_hwtstamps(adapter->ptp_tx_skb);	/* NULL skb */

Would extending ptp_lock over the whole read_hwstamp sequence, including
the non-NULL test, or not touching the shared pointer from these error
paths at all, address both this and the reference-drop race above?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917105713.2147903-1-vulab%40iscas.ac.cn