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

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

[Jiawen Wu]

wx->ptp_tx_skb is shared between the Tx path, the PTP auxiliary
worker and the timestamp cleanup paths. The
WX_STATE_PTP_TX_IN_PROGRESS bit prevents multiple Tx paths from
submitting timestamp requests, but does not serialize the worker
against cleanup.

As a result, wx_ptp_clear_tx_timestamp() can free an skb after
wx_ptp_tx_hwtstamp_work() has obtained its pointer. The worker may
then pass the freed skb to skb_tstamp_tx() and release the same
reference again.

The cleanup path may also clear the in-progress bit while the worker
is still processing the old skb. This allows the Tx path to publish a
new skb which the worker can subsequently overwrite with NULL,
leaking its reference.

Add a dedicated spinlock to protect publication and consumption of
the Tx timestamp skb. Detach the skb and clear the in-progress bit
while holding the lock, then deliver the timestamp and release the skb
after dropping it. Use the same locked cleanup in the quiesce path.
The lock is taken with interrupts disabled, because netpoll can call
ndo_start_xmit() with hard interrupts already off.

When handling a Tx DMA mapping failure, keep the transmit path
reference until after comparing the skb under the lock. This prevents
skb address reuse from making the error path mistake a newer timestamp
request for the failed one.

Fixes: 06e75161b9d4 ("net: wangxun: Add support for PTP clock")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/6C7EC12D69217315%2B20260818074721.45536-1-jiawenwu%40trustnetic.com
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
v2:
- Keep the original skb reference alive until the PTP cleanup has compared
  it under the lock, preventing slab address reuse from matching and
  cancelling a newer timestamp request.
- Let the transmit caller release the skb after both DMA rollback and PTP
  cleanup have completed.
- Only cancel the failed timestamp request and increment
  tx_hwtstamp_errors when the outstanding skb still belongs to that
  transmit.
- Factor the common timestamp detach sequence into a locked helper,
  including unlatching the hardware timestamp, detaching ptp_tx_skb and
  clearing the in-progress state bit.
- Make wx_ptp_quiesce() use the common locked timestamp cleanup path.
- Use spin_lock_irqsave() for ptp_tx_lock because ndo_start_xmit() may be
  invoked by netpoll with hard interrupts disabled.
- Protect ptp_tx_skb, ptp_tx_start and the in-progress state bit with the
  same lock.
- Release or deliver detached skbs after dropping the lock.
- Add comments and kernel-doc describing skb ownership, locking and
  timestamp worker return semantics.

v1: https://lore.kernel.org/all/8F4D34F6863177CC+20260908081142.86235-1-jiawenwu@trustnetic.com
---
 drivers/net/ethernet/wangxun/libwx/wx_hw.c   |   1 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c  |  46 +++++--
 drivers/net/ethernet/wangxun/libwx/wx_ptp.c  | 128 ++++++++++++-------
 drivers/net/ethernet/wangxun/libwx/wx_type.h |   2 +
 4 files changed, 115 insertions(+), 62 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 122c4952d203..113552586be7 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -2518,6 +2518,7 @@ int wx_sw_init(struct wx *wx)
 	}
 
 	spin_lock_init(&wx->hw_stats_lock);
+	spin_lock_init(&wx->ptp_tx_lock);
 	mutex_init(&wx->reset_lock);
 	bitmap_zero(wx->state, WX_STATE_NBITS);
 	bitmap_zero(wx->flags, WX_PF_FLAGS_NBITS);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index ed5aad7857bd..940ba2c6150f 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -1200,9 +1200,11 @@ static int wx_tx_map(struct wx_ring *tx_ring,
 		i--;
 	}
 
-	dev_kfree_skb_any(first->skb);
-	first->skb = NULL;
-
+	/* first->skb is released by the caller, which keeps a reference on it
+	 * until the PTP cleanup has compared it against wx->ptp_tx_skb. That
+	 * prevents the address from being reused by a newer request while the
+	 * comparison is pending.
+	 */
 	tx_ring->next_to_use = i;
 
 	return -ENOMEM;
@@ -1649,9 +1651,11 @@ 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) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
 		if (wx->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !test_and_set_bit_lock(WX_STATE_PTP_TX_IN_PROGRESS,
-					   wx->state)) {
+		    !test_and_set_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= WX_TX_FLAGS_TSTAMP;
 			wx->ptp_tx_skb = skb_get(skb);
@@ -1659,6 +1663,7 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
 		} else {
 			wx->tx_hwtstamp_skipped++;
 		}
+		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
 	}
 
 	/* record initial flags and protocol */
@@ -1677,19 +1682,34 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
 		wx->atr(tx_ring, first, ptype);
 
 	if (wx_tx_map(tx_ring, first, hdr_len))
-		goto cleanup_tx_tstamp;
+		goto out_drop;
 
 	return NETDEV_TX_OK;
 out_drop:
-	dev_kfree_skb_any(first->skb);
-	first->skb = NULL;
-cleanup_tx_tstamp:
+	/* The hardware will never report a timestamp for a frame it did not
+	 * transmit, so drop the request. Only do so if it is still ours: the
+	 * PTP worker may already have completed it and a concurrent transmit
+	 * may have submitted a new one.
+	 */
 	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
-		dev_kfree_skb_any(wx->ptp_tx_skb);
-		wx->ptp_tx_skb = NULL;
-		wx->tx_hwtstamp_errors++;
-		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
+		struct sk_buff *ptp_tx_skb = NULL;
+		unsigned long flags;
+
+		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
+		if (wx->ptp_tx_skb == skb) {
+			ptp_tx_skb = wx->ptp_tx_skb;
+			wx->ptp_tx_skb = NULL;
+			clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
+		}
+		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
+
+		if (ptp_tx_skb) {
+			dev_kfree_skb_any(ptp_tx_skb);
+			wx->tx_hwtstamp_errors++;
+		}
 	}
+	dev_kfree_skb_any(first->skb);
+	first->skb = NULL;
 
 	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 4708e7f3958f..f6346d257250 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
@@ -129,6 +129,31 @@ static int wx_ptp_settime64(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+/**
+ * __wx_ptp_detach_tx_skb - detach the skb tracking the Tx timestamp request
+ * @wx: the private board structure
+ *
+ * Unlatch any timestamp left in the hardware registers, detach the skb of the
+ * outstanding request and release the in-progress bit, so that a new request
+ * can be submitted.
+ *
+ * Context: Expects wx->ptp_tx_lock to be held by the caller.
+ * Return: the detached skb, or NULL if no request was outstanding. The caller
+ * owns the returned reference and must release it once the lock is dropped.
+ */
+static struct sk_buff *__wx_ptp_detach_tx_skb(struct wx *wx)
+{
+	struct sk_buff *skb = wx->ptp_tx_skb;
+
+	lockdep_assert_held(&wx->ptp_tx_lock);
+
+	rd32ptp(wx, WX_TSC_1588_STMPH);
+	wx->ptp_tx_skb = NULL;
+	clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
+
+	return skb;
+}
+
 /**
  * wx_ptp_clear_tx_timestamp - utility function to clear Tx timestamp state
  * @wx: the private board structure
@@ -139,12 +164,14 @@ static int wx_ptp_settime64(struct ptp_clock_info *ptp,
  */
 static void wx_ptp_clear_tx_timestamp(struct wx *wx)
 {
-	rd32ptp(wx, WX_TSC_1588_STMPH);
-	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);
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wx->ptp_tx_lock, flags);
+	skb = __wx_ptp_detach_tx_skb(wx);
+	spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
+
+	dev_kfree_skb_any(skb);
 }
 
 /**
@@ -175,49 +202,53 @@ static void wx_ptp_convert_to_hwtstamp(struct wx *wx,
 }
 
 /**
- * wx_ptp_tx_hwtstamp - utility function which checks for TX time stamp
+ * wx_ptp_tx_hwtstamp_work - check for a pending 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
+ * If a Tx timestamp request is outstanding and the hardware has latched a
+ * valid value, we convert it into the timecounter ns value, then store that
+ * result into the shhwtstamps structure which is passed up the network stack.
+ *
+ * Return: 0 when there is nothing left to poll for, -1 when the timestamp is
+ * not available yet and the caller should poll again.
  */
-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;
+	unsigned long flags;
+	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_irqsave(&wx->ptp_tx_lock, flags);
 
 	/* we have to have a valid skb to poll for a timestamp */
 	if (!wx->ptp_tx_skb) {
-		wx_ptp_clear_tx_timestamp(wx);
+		__wx_ptp_detach_tx_skb(wx);
+		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
 		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_irqrestore(&wx->ptp_tx_lock, flags);
+		return -1;
 	}
 
-	return -1;
+	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPL);
+	regval |= (u64)rd32ptp(wx, WX_TSC_1588_STMPH) << 32;
+	skb = wx->ptp_tx_skb;
+	wx->ptp_tx_skb = NULL;
+	clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
+	spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
+
+	wx_ptp_convert_to_hwtstamp(wx, &shhwtstamps, regval);
+	skb_tstamp_tx(skb, &shhwtstamps);
+	dev_kfree_skb_any(skb);
+	wx->tx_hwtstamp_pkts++;
+
+	return 0;
 }
 
 /**
@@ -296,24 +327,27 @@ static void wx_ptp_rx_hang(struct wx *wx)
  */
 static void wx_ptp_tx_hang(struct wx *wx)
 {
-	bool timeout = time_is_before_jiffies(wx->ptp_tx_start +
-					      WX_PTP_TX_TIMEOUT);
-
-	if (!wx->ptp_tx_skb)
-		return;
+	struct sk_buff *skb = NULL;
+	unsigned long flags;
 
-	if (!test_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state))
-		return;
+	spin_lock_irqsave(&wx->ptp_tx_lock, flags);
 
 	/* If we haven't received a timestamp within the timeout, it is
 	 * reasonable to assume that it will never occur, so we can unlock the
 	 * timestamp bit when this occurs.
 	 */
-	if (timeout) {
-		wx_ptp_clear_tx_timestamp(wx);
-		wx->tx_hwtstamp_timeouts++;
-		dev_warn(&wx->pdev->dev, "clearing Tx timestamp hang\n");
-	}
+	if (wx->ptp_tx_skb &&
+	    test_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state) &&
+	    time_is_before_jiffies(wx->ptp_tx_start + WX_PTP_TX_TIMEOUT))
+		skb = __wx_ptp_detach_tx_skb(wx);
+	spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
+
+	if (!skb)
+		return;
+
+	dev_kfree_skb_any(skb);
+	wx->tx_hwtstamp_timeouts++;
+	dev_warn(&wx->pdev->dev, "clearing Tx timestamp hang\n");
 }
 
 static long wx_ptp_do_aux_work(struct ptp_clock_info *ptp)
@@ -849,11 +883,7 @@ void wx_ptp_quiesce(struct wx *wx)
 	if (wx->ptp_clock)
 		ptp_cancel_worker_sync(wx->ptp_clock);
 
-	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);
+	wx_ptp_clear_tx_timestamp(wx);
 
 	if (wx->ptp_clock) {
 		ptp_clock_unregister(wx->ptp_clock);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 9454e90258d8..afd980dbb793 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1430,6 +1430,8 @@ struct wx {
 	unsigned long last_overflow_check;
 	unsigned long last_rx_ptp_check;
 	unsigned long ptp_tx_start;
+	/* protects ptp_tx_skb, ptp_tx_start and the in-progress state bit */
+	spinlock_t ptp_tx_lock;
 	seqlock_t hw_tc_lock; /* seqlock for ptp */
 	struct cyclecounter hw_cc;
 	struct timecounter hw_tc;
-- 
2.51.0

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

[Jacob Keller]

On 9/14/2026 1:00 AM, Jiawen Wu wrote:
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index ed5aad7857bd..940ba2c6150f 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -1677,19 +1682,34 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  		wx->atr(tx_ring, first, ptype);
>  
>  	if (wx_tx_map(tx_ring, first, hdr_len))
> -		goto cleanup_tx_tstamp;
> +		goto out_drop;
>  
>  	return NETDEV_TX_OK;
>  out_drop:
> -	dev_kfree_skb_any(first->skb);
> -	first->skb = NULL;
> -cleanup_tx_tstamp:
> +	/* The hardware will never report a timestamp for a frame it did not
> +	 * transmit, so drop the request. Only do so if it is still ours: the
> +	 * PTP worker may already have completed it and a concurrent transmit
> +	 * may have submitted a new one.
> +	 */

Could you explain this a bit more? The hardware doesn't report a
timestamp if the frame didn't transmit.. but you say here the PTP worker
may have already cleaned this up? How goes that work here? I guess
clean_tx_tstamp label may execute even if the timestamp has actually
happened and already completed? I'm not quite following this logic.

>  	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> -		dev_kfree_skb_any(wx->ptp_tx_skb);
> -		wx->ptp_tx_skb = NULL;
> -		wx->tx_hwtstamp_errors++;
> -		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +		struct sk_buff *ptp_tx_skb = NULL;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
> +		if (wx->ptp_tx_skb == skb) {
> +			ptp_tx_skb = wx->ptp_tx_skb;
> +			wx->ptp_tx_skb = NULL;
> +			clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +		}
> +		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
> +
> +		if (ptp_tx_skb) {
> +			dev_kfree_skb_any(ptp_tx_skb);
> +			wx->tx_hwtstamp_errors++;
> +		}
>  	}
> +	dev_kfree_skb_any(first->skb);
> +	first->skb = NULL;
>  
>  	return NETDEV_TX_OK;
>  }

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

[Jiawen Wu]

On Wed, Sep 16, 2026 7:51 AM, Jacob Keller wrote:
> On 9/14/2026 1:00 AM, Jiawen Wu wrote:
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index ed5aad7857bd..940ba2c6150f 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -1677,19 +1682,34 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
> >  		wx->atr(tx_ring, first, ptype);
> >
> >  	if (wx_tx_map(tx_ring, first, hdr_len))
> > -		goto cleanup_tx_tstamp;
> > +		goto out_drop;
> >
> >  	return NETDEV_TX_OK;
> >  out_drop:
> > -	dev_kfree_skb_any(first->skb);
> > -	first->skb = NULL;
> > -cleanup_tx_tstamp:
> > +	/* The hardware will never report a timestamp for a frame it did not
> > +	 * transmit, so drop the request. Only do so if it is still ours: the
> > +	 * PTP worker may already have completed it and a concurrent transmit
> > +	 * may have submitted a new one.
> > +	 */
> 
> Could you explain this a bit more? The hardware doesn't report a
> timestamp if the frame didn't transmit.. but you say here the PTP worker
> may have already cleaned this up? How goes that work here? I guess
> clean_tx_tstamp label may execute even if the timestamp has actually
> happened and already completed? I'm not quite following this logic.

I think I was somewhat misled by the AI, the comment is wrong.

The label is not reachable for a frame what was actually transmitted.
It has only two entries, and both are before the frame is handed to the
hardware. No timestamp can be latched for such a frame, so the request
has to be cancelled. That part of the comment is accurate.

What is wrong is the reason I gave for the ownership check. The PTP
worker cannot have completed the request for a frame that was never
transmitted - it only consumes the slot when WX_TSC_1588_CTL_VALID is
set, and there is no new latch without a transmission. Bringing the
worker in there is simply incorrect.

I'll fix the comment like this:

/* The frame never reached the hardware, so no timestamp will ever be reported
 * for it and the request has to be cancelled. The slot is shared, though:
 * wx_ptp_clear_tx_timestamp() or wx_ptp_tx_hang() may have dropped our request
 * request already, and a transmit on another queue can have claimed the slot
 * since. Only cancel it while it is still ours, otherwise we would free
 * somebody else's skb and release their in-progress bit.
 */

> 
> >  	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> > -		dev_kfree_skb_any(wx->ptp_tx_skb);
> > -		wx->ptp_tx_skb = NULL;
> > -		wx->tx_hwtstamp_errors++;
> > -		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > +		struct sk_buff *ptp_tx_skb = NULL;
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
> > +		if (wx->ptp_tx_skb == skb) {
> > +			ptp_tx_skb = wx->ptp_tx_skb;
> > +			wx->ptp_tx_skb = NULL;
> > +			clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > +		}
> > +		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
> > +
> > +		if (ptp_tx_skb) {
> > +			dev_kfree_skb_any(ptp_tx_skb);
> > +			wx->tx_hwtstamp_errors++;
> > +		}
> >  	}
> > +	dev_kfree_skb_any(first->skb);
> > +	first->skb = NULL;
> >
> >  	return NETDEV_TX_OK;
> >  }
> 

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

[netdev-bot+sashiko]

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

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

- [Medium] The patch makes wx_ptp_quiesce() (wx_ptp.c:886) perform an
  MMIO register read on a device whose PCIe channel may be frozen or…

Pre-existing issues:
- [High] The NAPI Tx cleanup path can call ptp_schedule_worker() on a PTP
  clock that wx_ptp_stop() is concurrently unregistering and NULLing.
- [Low] wx_ptp_suspend()
  (drivers/net/ethernet/wangxun/libwx/wx_ptp.c:841-852) tears down the…
- [Low] wx_xmit_frame_ring() reads wx->tstamp_config.tx_type inside the
  new ptp_tx_lock critical section (wx_lib.c:1656-1658), but the writer…
- [Low] In wx_xmit_frame_ring()'s out_drop path, wx->tx_hwtstamp_errors++
  is executed after dropping ptp_tx_lock (wx_lib.c:1706-1709), i.e.…

--

net: libwx: fix races in Tx timestamp handling

Adds a dedicated spinlock (wx->ptp_tx_lock) that serializes publication and
consumption of wx->ptp_tx_skb between the Tx path, the PTP auxiliary worker
and the timestamp cleanup paths.  The detach sequence is factored into
__wx_ptp_detach_tx_skb(), and the Tx error path now compares the outstanding
request against its own skb before cancelling it.

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 ed5aad7857bd..940ba2c6150f 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c

[ ... ]

> @@ -1649,9 +1651,11 @@ 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) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
>  		if (wx->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
> -		    !test_and_set_bit_lock(WX_STATE_PTP_TX_IN_PROGRESS,
> -					   wx->state)) {
> +		    !test_and_set_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state)) {

[Severity: Low]
This is a pre-existing inconsistency and not something introduced here, but
is the read of wx->tstamp_config.tx_type actually covered by the new lock?

The writer side never takes ptp_tx_lock, and it publishes the new config
only after the hardware has already been reprogrammed and cleaned up:

wx_hwtstamp_set()
    wx_ptp_set_timestamp_mode(wx, cfg);   /* clears WX_TSC_1588_CTL_ENABLED,
                                             calls wx_ptp_clear_tx_timestamp() */
    memcpy(&wx->tstamp_config, cfg, sizeof(wx->tstamp_config));

Can a transmit on another CPU still observe HWTSTAMP_TX_ON in that window,
arm WX_STATE_PTP_TX_IN_PROGRESS and set SKBTX_IN_PROGRESS for a frame the
hardware will never timestamp?  The request then pins the skb and its
socket until wx_ptp_tx_hang() cancels it (WX_PTP_TX_TIMEOUT is HZ) with a
bogus tx_hwtstamp_timeouts increment and a "clearing Tx timestamp hang"
warning.  Would publishing tx_type under ptp_tx_lock close that?

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

[Severity: Low]
This isn't a bug introduced by this patch either, but can this publication
outlive its only consumer on the suspend path?

wx_ptp_suspend() clears WX_STATE_PTP_RUNNING first, so wx_ptp_do_aux_work()
returns HZ without ever running wx_ptp_tx_hwtstamp_work() or
wx_ptp_tx_hang() again, and then performs the final cleanup while
wx->ptp_clock is still set and Tx has not been stopped yet:

txgbe_close_suspend()
    wx_ptp_suspend(wx);          /* clears RUNNING, wx_ptp_clear_tx_timestamp() */
    txgbe_disable_device(wx);    /* netif_tx_stop_all_queues()/netif_tx_disable() */
    wx_free_resources(wx);

Does an in-flight wx_xmit_frame_ring() that is already past the
queue-stopped check then leak the skb_get() reference (and the socket it
pins), with WX_STATE_PTP_TX_IN_PROGRESS left set and nothing left to
consume or time out the request?

On the PM path this is recovered later by resume -> ndo_open ->
wx_ptp_init() -> wx_ptp_reset() -> wx_ptp_set_timestamp_mode() ->
wx_ptp_clear_tx_timestamp(), so it appears to persist only on the
poweroff/shutdown path.  The ndo_stop path looks unaffected, since
dev_deactivate_many() and netpoll_poll_disable() run before ndo_stop.

>  @@ -1659,6 +1663,7 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  		} else {
>  			wx->tx_hwtstamp_skipped++;
>  		}
> +		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
>  	}
>  
>  	/* record initial flags and protocol */
> @@ -1677,19 +1682,34 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  		wx->atr(tx_ring, first, ptype);
>  
>  	if (wx_tx_map(tx_ring, first, hdr_len))
> -		goto cleanup_tx_tstamp;
> +		goto out_drop;
>  
>  	return NETDEV_TX_OK;
>  out_drop:
> -	dev_kfree_skb_any(first->skb);
> -	first->skb = NULL;
> -cleanup_tx_tstamp:
> +	/* The hardware will never report a timestamp for a frame it did not
> +	 * transmit, so drop the request. Only do so if it is still ours: the
> +	 * PTP worker may already have completed it and a concurrent transmit
> +	 * may have submitted a new one.
> +	 */
>  	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> -		dev_kfree_skb_any(wx->ptp_tx_skb);
> -		wx->ptp_tx_skb = NULL;
> -		wx->tx_hwtstamp_errors++;
> -		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +		struct sk_buff *ptp_tx_skb = NULL;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
> +		if (wx->ptp_tx_skb == skb) {
> +			ptp_tx_skb = wx->ptp_tx_skb;
> +			wx->ptp_tx_skb = NULL;
> +			clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +		}
> +		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
> +
> +		if (ptp_tx_skb) {
> +			dev_kfree_skb_any(ptp_tx_skb);
> +			wx->tx_hwtstamp_errors++;
> +		}
>  	}

[Severity: Low]
This is a pre-existing pattern rather than something the patch changes, but
could wx->tx_hwtstamp_errors++ move inside the critical section that is
already held just above?

It is a non-atomic read-modify-write performed after dropping ptp_tx_lock,
and two transmits on different netdev queues share no HARD_TX_LOCK, so can
one increment be lost and make ts_stats->err in wx_get_ts_stats() undercount?
tx_hwtstamp_skipped is incremented under the lock in the arming hunk above.

> +	dev_kfree_skb_any(first->skb);
> +	first->skb = NULL;
>  
>  	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 4708e7f3958f..f6346d257250 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> @@ -129,6 +129,31 @@ static int wx_ptp_settime64(struct ptp_clock_info *ptp,
> +static struct sk_buff *__wx_ptp_detach_tx_skb(struct wx *wx)
> +{
> +	struct sk_buff *skb = wx->ptp_tx_skb;
> +
> +	lockdep_assert_held(&wx->ptp_tx_lock);
> +
> +	rd32ptp(wx, WX_TSC_1588_STMPH);
> +	wx->ptp_tx_skb = NULL;
> +	clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> +
> +	return skb;
> +}

[Severity: Medium]
Is this rd32ptp() safe on the quiesce path?  With this patch,
wx_ptp_quiesce() reaches it through wx_ptp_clear_tx_timestamp(), and
wx_ptp_quiesce() only runs while the PCIe channel is known to be in
trouble:

wx_io_error_detected()          /* including pci_channel_io_frozen and
                                   pci_channel_io_perm_failure */
    set_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
    wx_soft_quiesce(wx)
        wx_ptp_quiesce(wx)
            wx_ptp_clear_tx_timestamp(wx)
                __wx_ptp_detach_tx_skb(wx)
                    rd32ptp(wx, WX_TSC_1588_STMPH);   /* readl() */

wx_reset_task() takes the same route after wx_check_pcie_error() has
already decided the link or memory space is gone.

Before this patch wx_ptp_quiesce() performed no register access at all
(it only freed wx->ptp_tx_skb and cleared the state bit), which also
matches commit c023e9769de9 ("net: wangxun: implement soft quiesce for
PCIe error recovery"):

    Function wx_soft_quiesce() provide a lightweight shutdown path during
    PCIe error recovery. It avoids MMIO-dependent operations in PCIe error
    status.

Since the read value is discarded and a dead device returns all-ones, the
unlatch this read exists for does not happen anyway.  Would an MMIO-free
detach for the quiesce path (take ptp_tx_lock, clear wx->ptp_tx_skb and
WX_STATE_PTP_TX_IN_PROGRESS) preserve that property, leaving the
WX_TSC_1588_STMPH unlatch to wx_ptp_suspend(), wx_ptp_tx_hang() and the
worker where the device is known to be alive?

>  /**
>   * wx_ptp_clear_tx_timestamp - utility function to clear Tx timestamp state
>   * @wx: the private board structure
> @@ -139,12 +164,14 @@ static int wx_ptp_settime64(struct ptp_clock_info *ptp,
>   */
>  static void wx_ptp_clear_tx_timestamp(struct wx *wx)
>  {
> -	rd32ptp(wx, WX_TSC_1588_STMPH);
> -	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);
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wx->ptp_tx_lock, flags);
> +	skb = __wx_ptp_detach_tx_skb(wx);
> +	spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
> +
> +	dev_kfree_skb_any(skb);
>  }

[ ... ]

> @@ -849,11 +883,7 @@ void wx_ptp_quiesce(struct wx *wx)
>  	if (wx->ptp_clock)
>  		ptp_cancel_worker_sync(wx->ptp_clock);
>  
> -	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);
> +	wx_ptp_clear_tx_timestamp(wx);
>  
>  	if (wx->ptp_clock) {
>  		ptp_clock_unregister(wx->ptp_clock);

[Severity: High]
This isn't a bug introduced by this patch, since wx_clean_tx_irq() and the
close ordering are untouched, but while the locking of the in-progress bit
is being reworked, is wx->ptp_clock safe against the NAPI Tx cleanup path?

wx_clean_tx_irq() reads the state bit without ptp_tx_lock and then hands
wx->ptp_clock to the scheduler:

	/* schedule check for Tx timestamp */
	if (unlikely(test_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state)) &&
	    skb_shinfo(tx_buffer->skb)->tx_flags & SKBTX_IN_PROGRESS)
		ptp_schedule_worker(wx->ptp_clock, 0);

and the callee dereferences it right away:

drivers/ptp/ptp_clock.c:ptp_schedule_worker() {
	return kthread_mod_delayed_work(ptp->kworker, &ptp->aux_work, delay);
}

The same clock teardown as in wx_ptp_quiesce() above is done by
wx_ptp_stop(), which runs before NAPI is disabled:

txgbe_close()
    wx_ptp_stop(wx);          /* wx_ptp_suspend(), ptp_clock_unregister(),
                                 wx->ptp_clock = NULL */
    txgbe_down(wx);
        txgbe_disable_device(wx);
            wx_napi_disable_all(wx);

ngbe_close() has the same wx_ptp_stop() before ngbe_down() ordering, and
netif_carrier_off() also happens only inside the disable step, so the
early "if (!netif_carrier_ok())" return in wx_clean_tx_irq() does not
help here.

Can a poll that already passed the test_bit() check then pass a freed or
NULL wx->ptp_clock to ptp_schedule_worker() and oops in softirq context?
Should NAPI be disabled before ptp_clock_unregister(), or should the clock
pointer used by the Tx cleanup path be synchronized with teardown?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/73D0D3F5D96A6928%2B20260914080020.211580-1-jiawenwu%40trustnetic.com