[PATCH net v2] net: libwx: fix concurrent bitmap overwrite in PTP setup

[PATCH net v2] net: libwx: fix concurrent bitmap overwrite in PTP setup

[Jiawen Wu]

In wx_ptp_set_timestamp_mode(), the driver copies the global `wx->flags`
bitmap to a local variable, modifies the PTP-related bits, and then writes
the entire bitmap back using memcpy().

This Read-Copy-Update pattern is unsafe and introduces a critical race
condition. Other asynchronous contexts (such as Tx timeout routines or
GPIO IRQ handlers) update individual bits in `wx->flags` concurrently
using atomic bitops like set_bit() or clear_bit(). The memcpy() write-back
can silently overwrite and drop these concurrent changes, potentially
causing the driver to miss critical module reset or PCIe recovery requests.

Fix this by removing the local bitmap copy. Instead, evaluate the intended
PTP flag states locally and apply them directly to `wx->flags` using
atomic set_bit() and clear_bit() operations only after the hardware is
successfully configured.

Fixes: 06e75161b9d4 ("net: wangxun: Add support for PTP clock")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
Changes in v2:
- Combine two variables that are always equal into one.
- Link to v1: https://lore.kernel.org/all/F97867A3AA2696FD+20260817021708.37975-1-jiawenwu@trustnetic.com
---
 drivers/net/ethernet/wangxun/libwx/wx_ptp.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
index 44f3e6505246..1165518d5522 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
@@ -555,13 +555,11 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
 {
 	u32 tsync_tx_ctl = WX_TSC_1588_CTL_ENABLED;
 	u32 tsync_rx_ctl = WX_PSR_1588_CTL_ENABLED;
-	DECLARE_BITMAP(flags, WX_PF_FLAGS_NBITS);
 	u32 tsync_rx_mtrl = PTP_EV_PORT << 16;
+	bool rx_tstamp = false;
 	bool is_l2 = false;
 	u32 regval;
 
-	memcpy(flags, wx->flags, sizeof(wx->flags));
-
 	switch (config->tx_type) {
 	case HWTSTAMP_TX_OFF:
 		tsync_tx_ctl = 0;
@@ -576,20 +574,16 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
 	case HWTSTAMP_FILTER_NONE:
 		tsync_rx_ctl = 0;
 		tsync_rx_mtrl = 0;
-		clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
-		clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
 		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
 		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_SYNC;
-		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
-		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
+		rx_tstamp = true;
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
 		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
 		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_DELAY_REQ;
-		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
-		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
+		rx_tstamp = true;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
@@ -602,9 +596,8 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
 		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_EVENT_V2;
 		is_l2 = true;
+		rx_tstamp = true;
 		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
-		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
 		break;
 	default:
 		/* register PSR_1588_MSG must be set in order to do V1 packets,
@@ -643,7 +636,8 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
 	WX_WRITE_FLUSH(wx);
 
 	/* configure adapter flags only when HW is actually configured */
-	memcpy(wx->flags, flags, sizeof(wx->flags));
+	assign_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, wx->flags, rx_tstamp);
+	assign_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, wx->flags, rx_tstamp);
 
 	/* clear TX/RX timestamp state, just to be sure */
 	wx_ptp_clear_tx_timestamp(wx);
-- 
2.51.0

Re: [PATCH net v2] net: libwx: fix concurrent bitmap overwrite in PTP setup

[Vadim Fedorenko]

On 18/08/2026 08:47, Jiawen Wu wrote:
> In wx_ptp_set_timestamp_mode(), the driver copies the global `wx->flags`
> bitmap to a local variable, modifies the PTP-related bits, and then writes
> the entire bitmap back using memcpy().
> 
> This Read-Copy-Update pattern is unsafe and introduces a critical race
> condition. Other asynchronous contexts (such as Tx timeout routines or
> GPIO IRQ handlers) update individual bits in `wx->flags` concurrently
> using atomic bitops like set_bit() or clear_bit(). The memcpy() write-back
> can silently overwrite and drop these concurrent changes, potentially
> causing the driver to miss critical module reset or PCIe recovery requests.
> 
> Fix this by removing the local bitmap copy. Instead, evaluate the intended
> PTP flag states locally and apply them directly to `wx->flags` using
> atomic set_bit() and clear_bit() operations only after the hardware is
> successfully configured.
> 
> Fixes: 06e75161b9d4 ("net: wangxun: Add support for PTP clock")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
> Changes in v2:
> - Combine two variables that are always equal into one.
> - Link to v1: https://lore.kernel.org/all/F97867A3AA2696FD+20260817021708.37975-1-jiawenwu@trustnetic.com
> ---
>   drivers/net/ethernet/wangxun/libwx/wx_ptp.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> index 44f3e6505246..1165518d5522 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> @@ -555,13 +555,11 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
>   {
>   	u32 tsync_tx_ctl = WX_TSC_1588_CTL_ENABLED;
>   	u32 tsync_rx_ctl = WX_PSR_1588_CTL_ENABLED;
> -	DECLARE_BITMAP(flags, WX_PF_FLAGS_NBITS);
>   	u32 tsync_rx_mtrl = PTP_EV_PORT << 16;
> +	bool rx_tstamp = false;
>   	bool is_l2 = false;
>   	u32 regval;
>   
> -	memcpy(flags, wx->flags, sizeof(wx->flags));
> -
>   	switch (config->tx_type) {
>   	case HWTSTAMP_TX_OFF:
>   		tsync_tx_ctl = 0;
> @@ -576,20 +574,16 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
>   	case HWTSTAMP_FILTER_NONE:
>   		tsync_rx_ctl = 0;
>   		tsync_rx_mtrl = 0;
> -		clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> -		clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
>   		break;
>   	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
>   		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
>   		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_SYNC;
> -		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> -		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> +		rx_tstamp = true;
>   		break;
>   	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
>   		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
>   		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_DELAY_REQ;
> -		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> -		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> +		rx_tstamp = true;
>   		break;
>   	case HWTSTAMP_FILTER_PTP_V2_EVENT:
>   	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> @@ -602,9 +596,8 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
>   	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>   		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_EVENT_V2;
>   		is_l2 = true;
> +		rx_tstamp = true;
>   		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> -		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> -		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
>   		break;
>   	default:
>   		/* register PSR_1588_MSG must be set in order to do V1 packets,
> @@ -643,7 +636,8 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
>   	WX_WRITE_FLUSH(wx);
>   
>   	/* configure adapter flags only when HW is actually configured */
> -	memcpy(wx->flags, flags, sizeof(wx->flags));
> +	assign_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, wx->flags, rx_tstamp);
> +	assign_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, wx->flags, rx_tstamp);
>   
>   	/* clear TX/RX timestamp state, just to be sure */
>   	wx_ptp_clear_tx_timestamp(wx);

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net v2] net: libwx: fix concurrent bitmap overwrite in PTP setup

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Looks unrelated enough to apply this patch but also
needs fixing"
---
net: libwx: fix concurrent bitmap overwrite in PTP setup

This replaces the read-copy-writeback of wx->flags in
wx_ptp_set_timestamp_mode() with direct atomic updates of the two PTP Rx
timestamp bits, so concurrent set_bit()/clear_bit() users of wx->flags are
no longer clobbered by the memcpy() write-back.

Link: https://lore.kernel.org/all/F97867A3AA2696FD+20260817021708.37975-1-jiawenwu@trustnetic.com

Two questions below, one on the changelog and one on the code the last
hunk touches.

> This Read-Copy-Update pattern is unsafe and introduces a critical race
> condition. Other asynchronous contexts (such as Tx timeout routines or
> GPIO IRQ handlers) update individual bits in `wx->flags` concurrently
> using atomic bitops like set_bit() or clear_bit(). The memcpy() write-back
> can silently overwrite and drop these concurrent changes, potentially
> causing the driver to miss critical module reset or PCIe recovery requests.

[Severity: Low]
Could this impact description be narrowed to what exists in the driver?

A grep for tx_timeout under drivers/net/ethernet/wangxun/ finds nothing,
and the reset entry points txgbe_do_reset()/txgbe_reinit_locked() are
reached from ethtool and ndo_set_features under rtnl and operate on
wx->state, not wx->flags:

drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:txgbe_reinit_locked() {
	netif_trans_update(wx->netdev);

	mutex_lock(&wx->reset_lock);
	set_bit(WX_STATE_RESETTING, wx->state);
	...
}

Similarly, there is no pci_error_handlers / io_error_detected / slot_reset
in the driver, and enum wx_pf_flags in wx_type.h has no PCIe recovery
request bit, so which flag would carry a lost "PCIe recovery request"?

The one asynchronous producer that does match the description is the hard
IRQ handler in txgbe_aml.c:

drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c:txgbe_gpio_irq_handler_aml() {
		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
}

consumed by txgbe_module_detection_subtask() with test_and_clear_bit().
Would it be more accurate to reference only that path?

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> index 44f3e6505246..1165518d5522 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c

[ ... ]

> @@ -643,7 +636,8 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
>  	WX_WRITE_FLUSH(wx);
>  
>  	/* configure adapter flags only when HW is actually configured */
> -	memcpy(wx->flags, flags, sizeof(wx->flags));
> +	assign_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, wx->flags, rx_tstamp);
> +	assign_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, wx->flags, rx_tstamp);
>  
>  	/* clear TX/RX timestamp state, just to be sure */
>  	wx_ptp_clear_tx_timestamp(wx);

[Severity: High]
This isn't a bug introduced by this patch, but while the flags handling in
wx_ptp_set_timestamp_mode() is being cleaned up, can the trailing
wx_ptp_clear_tx_timestamp(wx) call race with the ptp aux worker over
wx->ptp_tx_skb?

wx_ptp_set_timestamp_mode() runs from wx_hwtstamp_set() under rtnl only,
while wx_ptp_do_aux_work() -> wx_ptp_tx_hwtstamp_work() ->
wx_ptp_tx_hwtstamp() runs in the ptp kthread armed by
ptp_schedule_worker() in wx_ptp_reset() and takes no lock:

drivers/net/ethernet/wangxun/libwx/wx_ptp.c:wx_ptp_tx_hwtstamp() {
	struct sk_buff *skb = wx->ptp_tx_skb;
	...
	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);
}

In the window after skb is cached and before wx->ptp_tx_skb is cleared, the
configuration path can free the same pointer:

drivers/net/ethernet/wangxun/libwx/wx_ptp.c:wx_ptp_clear_tx_timestamp() {
	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);
}

Only one reference exists, taken in wx_lib.c:wx_xmit_frame_ring():

		wx->ptp_tx_skb = skb_get(skb);
		wx->ptp_tx_start = jiffies;

so does an SIOCSHWTSTAMP or ethtool TSCONFIG_SET while a Tx timestamp is
outstanding lead to two dev_kfree_skb_any() calls on that single
reference, with skb_tstamp_tx() then touching freed memory?

There is a second ordering concern in the same area: since
wx_ptp_clear_tx_timestamp() drops WX_STATE_PTP_TX_IN_PROGRESS while the
worker is still using the old skb, can wx_xmit_frame_ring() publish a new
wx->ptp_tx_skb that the worker then overwrites with NULL, leaking it?
WX_STATE_PTP_TX_IN_PROGRESS is set on entry and cleared on exit rather
than being a lock, so it does not appear to provide mutual exclusion
between the rtnl path and the ptp kthread. Would a lock (or cancelling
the aux work before reconfiguring) be a better fit here?

Re: [PATCH net v2] net: libwx: fix concurrent bitmap overwrite in PTP setup

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 18 Aug 2026 15:47:21 +0800 you wrote:
> In wx_ptp_set_timestamp_mode(), the driver copies the global `wx->flags`
> bitmap to a local variable, modifies the PTP-related bits, and then writes
> the entire bitmap back using memcpy().
> 
> This Read-Copy-Update pattern is unsafe and introduces a critical race
> condition. Other asynchronous contexts (such as Tx timeout routines or
> GPIO IRQ handlers) update individual bits in `wx->flags` concurrently
> using atomic bitops like set_bit() or clear_bit(). The memcpy() write-back
> can silently overwrite and drop these concurrent changes, potentially
> causing the driver to miss critical module reset or PCIe recovery requests.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: libwx: fix concurrent bitmap overwrite in PTP setup
    https://git.kernel.org/netdev/net/c/f05516dd7b86

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html