Re: [PATCH v2 1/2] wifi: iwlwifi: mvm: Fix GP2 to nanoseconds overflow on 32-bit

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (1) · 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] The series fix is incomplete for the mld op-mode.
- [Low] The commit author (From:) is `Zhan Xusheng
  <zhanxusheng1024@gmail.com>`, but the only Signed-off-by is `Zhan…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in mvm the
  shared PTP state in mvm->ptp_data (last_gp2, wrap_counter,…

--

wifi: iwlwifi: mvm: Fix GP2 to nanoseconds overflow on 32-bit

This patch casts the u32 GP2 microsecond timestamps to u64 before they are
multiplied by NSEC_PER_USEC. The affected functions are
iwl_mvm_ptp_get_adj_time(), iwl_mvm_ptp_adjfine() and
iwl_mvm_rx_fill_status().

On 32-bit kernels these products were computed in 32 bits. They wrapped
for GP2 values above about 4.295 s, which made the PTP clock jump.

> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

[Severity: Low]
This isn't a bug, but the author address (zhanxusheng1024@gmail.com) is
different from the Signed-off-by address (zhanxusheng@xiaomi.com).
checkpatch reports this as a From:/Signed-off-by: email address mismatch.

Could a Signed-off-by be added for the gmail address? Another option is a
From: line with the xiaomi address in the patch body. The follow-up mld
commit in this series has the same mismatch.

> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ptp.c b/drivers/net/wireless/intel/iwlwifi/mvm/ptp.c
> index 49dcb1388007b..5f33e2f8fb2a9 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/ptp.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ptp.c

[ ... ]

> @@ -259,7 +259,7 @@ static int iwl_mvm_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  	 */
>  	gp2 = iwl_mvm_get_systime(mvm);
>  	data->scale_update_adj_time_ns =
> -		iwl_mvm_ptp_get_adj_time(mvm, gp2 * NSEC_PER_USEC);
> +		iwl_mvm_ptp_get_adj_time(mvm, (u64)gp2 * NSEC_PER_USEC);
>  	data->scale_update_gp2 = gp2;
>  	data->wrap_counter = 0;
>  	data->delta = 0;

[Severity: Medium]
This is a pre-existing issue, and this patch did not introduce it. Is
mvm->ptp_data safe from concurrent readers here?

iwl_mvm_ptp_adjfine() updates scale_update_adj_time_ns, scale_update_gp2,
wrap_counter and delta while holding only mvm->mutex.

Some RX paths call iwl_mvm_ptp_get_adj_time() without taking any lock:

  iwl_mvm_rx_fill_status()                (NAPI, rx_ts_ptp && monitor_on)
  iwl_mvm_time_sync_msmt_event()          (RX_HANDLER_SYNC)
  iwl_mvm_time_sync_msmt_confirm_event()  (RX_HANDLER_SYNC)

That function also changes state through iwl_mvm_ptp_update_new_read():

	if (gp2 < mvm->ptp_data.last_gp2) {
		mvm->ptp_data.wrap_counter++;
		...
	}

	mvm->ptp_data.last_gp2 = gp2;

The writers that hold mvm->mutex are iwl_mvm_ptp_adjfine(),
iwl_mvm_ptp_adjtime(), iwl_mvm_ptp_work(), iwl_mvm_ptp_gettime() and
iwl_mvm_phc_get_crosstimestamp(). That mutex does not keep out softirq RX
on other CPUs.

These interleavings look possible:

- RX sees the new scale_update_gp2 before wrap_counter is reset to 0, so
  the timestamp is off by N * 2^32 us.

- Two RSS queues both run the non-atomic wrap_counter++ / last_gp2
  update, so a wrap is counted twice or lost.

- On 32-bit, the u64 fields scale_update_adj_time_ns, scaled_freq and
  delta can tear while adjfine or adjtime writes them.

The mld driver protects the same state with data->lock, using
spin_lock_bh() in mld/ptp.c. Does mvm need similar locking?

> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> index 7f0b4f5daa214..7a9a63a76f382 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> @@ -2017,7 +2017,9 @@ static void iwl_mvm_rx_fill_status(struct iwl_mvm *mvm,
>  
>  	if (mvm->rx_ts_ptp && mvm->monitor_on) {
>  		u64 adj_time =
> -			iwl_mvm_ptp_get_adj_time(mvm, phy_data->gp2_on_air_rise * NSEC_PER_USEC);
> +			iwl_mvm_ptp_get_adj_time(mvm,
> +						 (u64)phy_data->gp2_on_air_rise *
> +						 NSEC_PER_USEC);

[Severity: High]
Does the mld op-mode still have this overflow in its RX path?

The follow-up commit "wifi: iwlwifi: mld: Fix GP2 to nanoseconds overflow
on 32-bit" says it fixes the "Same problem as the mvm driver". However, it
only changes the two sites in mld/ptp.c, in iwl_mld_ptp_get_adj_time() and
iwl_mld_ptp_adjfine().

At the end of the series, iwl_mld_rx_fill_status() in mld/rx.c still has
no cast:

drivers/net/wireless/intel/iwlwifi/mld/rx.c:iwl_mld_rx_fill_status() {
    ...
		if (mld->monitor.ptp_time) {
			u64 adj_time =
				iwl_mld_ptp_get_adj_time(mld,
							 phy_data->gp2_on_air_rise *
							 NSEC_PER_USEC);
    ...
}

gp2_on_air_rise is a u32 in struct iwl_mld_rx_phy_data, and NSEC_PER_USEC
is 1000L. On 32-bit the product therefore wraps for any GP2 value above
about 4,294,967 us.

The truncated value is then passed to iwl_mld_ptp_update_new_read() as a
small gp2. It is usually more than IWL_PTP_WRAP_THRESHOLD_USEC below
last_gp2, so it looks like a wraparound:

		mld->ptp_data.wrap_counter++;

This would happen each time the truncated value wraps. It would also
happen each time gettime or crosstimestamp stores a correct, large
last_gp2.

That would make rx_status->mactime wrong. It would also shift later
iwl_mld_ptp_gettime() results by 2^32 us for each spurious wrap.

IWLMLD does not depend on 64BIT, and this path is reachable with the
rx_ts_ptp debugfs knob and a monitor interface. Should the mld follow-up
also cast gp2_on_air_rise to u64 in iwl_mld_rx_fill_status()?

>  
>  		rx_status->mactime = div64_u64(adj_time, NSEC_PER_USEC);
>  		rx_status->flag |= RX_FLAG_MACTIME_IS_RTAP_TS64;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001050103.860584-1-zhanxusheng%40xiaomi.com