Re: [PATCH net] net: libwx: fix races in Tx timestamp handling

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Incomplete locking conversion: wx_ptp_quiesce() still
  detaches/frees wx->ptp_tx_skb and clears WX_STATE_PTP_TX_IN_PROGRESS…
- [Medium] The new identity check `if (wx->ptp_tx_skb == skb)` in
  wx_xmit_frame_ring()'s cleanup_tx_tstamp path (wx_lib.c:1692) compares…

--

net: libwx: fix races in Tx timestamp handling

This adds a dedicated spinlock (wx->ptp_tx_lock) to serialize publication
and consumption of wx->ptp_tx_skb between the Tx path, the PTP auxiliary
worker and the timestamp cleanup paths.  The skb is detached and the
WX_STATE_PTP_TX_IN_PROGRESS bit cleared under the lock, with delivery and
release done after unlocking.  The Tx DMA mapping error path also gains an
skb identity check.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/6C7EC12D69217315%2B20260818074721.45536-1-jiawenwu%40trustnetic.com

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index ed5aad7857bd9..9118813334a9e 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c

[ ... ]

> @@ -1649,6 +1650,7 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  
>  	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
>  	    wx->ptp_clock) {
> +		spin_lock_bh(&wx->ptp_tx_lock);
>  		if (wx->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
>  		    !test_and_set_bit_lock(WX_STATE_PTP_TX_IN_PROGRESS,
>  					   wx->state)) {
> @@ -1659,6 +1661,7 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  		} else {
>  			wx->tx_hwtstamp_skipped++;
>  		}
> +		spin_unlock_bh(&wx->ptp_tx_lock);
>  	}
>  
>  	/* record initial flags and protocol */
> @@ -1685,10 +1688,17 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  	first->skb = NULL;
>  cleanup_tx_tstamp:
>  	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> -		dev_kfree_skb_any(wx->ptp_tx_skb);
> -		wx->ptp_tx_skb = NULL;
> +		spin_lock_bh(&wx->ptp_tx_lock);
> +		if (wx->ptp_tx_skb == skb) {

[Severity: Medium]
Can this comparison match a different skb that happens to reuse the same
slab address?

By the time cleanup_tx_tstamp runs after a wx_tx_map() failure, this
function no longer holds any reference on skb.  The PTP branch above took
one via wx->ptp_tx_skb = skb_get(skb), but wx_tx_map()'s dma_error path
drops the ring reference before returning:

drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_tx_map() {
	...
dma_error:
	...
	dev_kfree_skb_any(first->skb);
	first->skb = NULL;
	tx_ring->next_to_use = i;

	return -ENOMEM;
}

So the only remaining reference on skb is the one held by wx->ptp_tx_skb,
and the local skb variable is just a bare pointer value.

If the PTP aux worker on another CPU consumes that request before this CPU
takes ptp_tx_lock:

CPU1 wx_ptp_tx_hwtstamp_work()
	skb = wx->ptp_tx_skb;
	wx->ptp_tx_skb = NULL;
	clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
	spin_unlock_bh(&wx->ptp_tx_lock);
	skb_tstamp_tx(skb, &shhwtstamps);
	dev_kfree_skb_any(skb);		<-- last reference gone

CPU2 wx_xmit_frame_ring()
	wx->ptp_tx_skb = skb_get(skb);	<-- new skb on the recycled object

CPU0 then reaches the compare with a stale pointer whose value now names
the newer request, so wx->ptp_tx_skb == skb is true and the newer request
is detached and cancelled, with WX_STATE_PTP_TX_IN_PROGRESS cleared and
tx_hwtstamp_errors charged for it.  skb_tstamp_tx() never runs for that
packet and SKBTX_IN_PROGRESS stays set.

Would something recycle-proof work better here, for example keeping the
local reference across the compare, or a per-request generation counter
instead of the pointer value?

The commit message says the change makes it "so that the error path cannot
clear a newer timestamp request" - does the pointer comparison alone
achieve that?

> +			ptp_tx_skb = wx->ptp_tx_skb;
> +			wx->ptp_tx_skb = NULL;
> +			clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS,
> +					 wx->state);
> +		}
> +		spin_unlock_bh(&wx->ptp_tx_lock);
> +
> +		dev_kfree_skb_any(ptp_tx_skb);
>  		wx->tx_hwtstamp_errors++;
> -		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
>  	}
>  
>  	return NETDEV_TX_OK;
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> index 4708e7f3958f7..a0929b4844e8d 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> @@ -139,12 +139,16 @@ static int wx_ptp_settime64(struct ptp_clock_info *ptp,
>   */
>  static void wx_ptp_clear_tx_timestamp(struct wx *wx)
>  {
> +	struct sk_buff *skb;
> +
> +	spin_lock_bh(&wx->ptp_tx_lock);
>  	rd32ptp(wx, WX_TSC_1588_STMPH);
> -	if (wx->ptp_tx_skb) {
> -		dev_kfree_skb_any(wx->ptp_tx_skb);
> -		wx->ptp_tx_skb = NULL;
> -	}
> +	skb = wx->ptp_tx_skb;
> +	wx->ptp_tx_skb = NULL;
>  	clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +	spin_unlock_bh(&wx->ptp_tx_lock);
> +
> +	dev_kfree_skb_any(skb);
>  }

[Severity: Medium]
Was wx_ptp_quiesce() intentionally left out of this conversion?

It is the one remaining place that detaches and frees wx->ptp_tx_skb and
clears WX_STATE_PTP_TX_IN_PROGRESS without taking the new lock:

drivers/net/ethernet/wangxun/libwx/wx_ptp.c:wx_ptp_quiesce() {
	...
	if (wx->ptp_tx_skb) {
		dev_kfree_skb_any(wx->ptp_tx_skb);
		wx->ptp_tx_skb = NULL;
	}
	clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
	...
}

That is also the pattern this hunk replaces, i.e. freeing under the same
sequence the other three sites now perform while holding ptp_tx_lock, and
it skips the rd32ptp(wx, WX_TSC_1588_STMPH) unlatch read that
wx_ptp_clear_tx_timestamp(), wx_ptp_tx_hwtstamp_work() and wx_ptp_tx_hang()
all do.

Today the only caller is wx_soft_quiesce(), which does netif_tx_disable()
and wx_napi_disable_all() first, and wx_ptp_quiesce() itself does
ptp_cancel_worker_sync() before touching the pointer, so I could not build
a concurrent execution against the locked consumers.  But wx_ptp_quiesce()
is exported, and the exclusion depends entirely on that call ordering
rather than on the lock the patch adds.

Could this site just call wx_ptp_clear_tx_timestamp(), or use a factored
unlocked helper plus a locked wrapper, so all four paths share one
sequence?

> [ ... ]

> @@ -174,50 +178,43 @@ static void wx_ptp_convert_to_hwtstamp(struct wx *wx,
>  	hwtstamp->hwtstamp = ns_to_ktime(ns);
>  }
>  
> -/**
> - * wx_ptp_tx_hwtstamp - utility function which checks for TX time stamp
> - * @wx: the private board struct
> - *
> - * if the timestamp is valid, we convert it into the timecounter ns
> - * value, then store that result into the shhwtstamps structure which
> - * is passed up the network stack
> - */
> -static void wx_ptp_tx_hwtstamp(struct wx *wx)
> +static int wx_ptp_tx_hwtstamp_work(struct wx *wx)
>  {
>  	struct skb_shared_hwtstamps shhwtstamps;
> -	struct sk_buff *skb = wx->ptp_tx_skb;
> +	struct sk_buff *skb;
> +	u32 tsynctxctl;
>  	u64 regval = 0;
>  
> -	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPL);
> -	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPH) << 32;
> -
> -	wx_ptp_convert_to_hwtstamp(wx, &shhwtstamps, regval);
> -
> -	wx->ptp_tx_skb = NULL;
> -	clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> -	skb_tstamp_tx(skb, &shhwtstamps);
> -	dev_kfree_skb_any(skb);
> -	wx->tx_hwtstamp_pkts++;
> -}
> -
> -static int wx_ptp_tx_hwtstamp_work(struct wx *wx)
> -{
> -	u32 tsynctxctl;
> +	spin_lock_bh(&wx->ptp_tx_lock);
>  
>  	/* we have to have a valid skb to poll for a timestamp */
>  	if (!wx->ptp_tx_skb) {
> -		wx_ptp_clear_tx_timestamp(wx);
> +		rd32ptp(wx, WX_TSC_1588_STMPH);
> +		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +		spin_unlock_bh(&wx->ptp_tx_lock);
>  		return 0;
>  	}
>  
>  	/* stop polling once we have a valid timestamp */
>  	tsynctxctl = rd32ptp(wx, WX_TSC_1588_CTL);
> -	if (tsynctxctl & WX_TSC_1588_CTL_VALID) {
> -		wx_ptp_tx_hwtstamp(wx);
> -		return 0;
> +	if (!(tsynctxctl & WX_TSC_1588_CTL_VALID)) {
> +		spin_unlock_bh(&wx->ptp_tx_lock);
> +		return -1;
>  	}
>  
> -	return -1;
> +	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPL);
> +	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPH) << 32;
> +	wx_ptp_convert_to_hwtstamp(wx, &shhwtstamps, regval);
> +	skb = wx->ptp_tx_skb;
> +	wx->ptp_tx_skb = NULL;
> +	clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +	spin_unlock_bh(&wx->ptp_tx_lock);
> +
> +	skb_tstamp_tx(skb, &shhwtstamps);
> +	dev_kfree_skb_any(skb);
> +	wx->tx_hwtstamp_pkts++;
> +
> +	return 0;
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 9454e90258d8e..6b9460147ca93 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -1430,6 +1430,7 @@ struct wx {
>  	unsigned long last_overflow_check;
>  	unsigned long last_rx_ptp_check;
>  	unsigned long ptp_tx_start;
> +	spinlock_t ptp_tx_lock; /* protects ptp_tx_skb and ptp_tx_start */

[Severity: Medium]
This comment states the lock protects ptp_tx_skb, but wx_ptp_quiesce()
still reads, frees and NULLs wx->ptp_tx_skb without it, as noted above.
Should the comment be narrowed, or wx_ptp_quiesce() updated to match it?

>  	seqlock_t hw_tc_lock; /* seqlock for ptp */
>  	struct cyclecounter hw_cc;
>  	struct timecounter hw_tc;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/8F4D34F6863177CC%2B20260908081142.86235-1-jiawenwu%40trustnetic.com