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

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete: one u32 GP2 × NSEC_PER_USEC
  multiplication in the mld driver is still unfixed.
- [Low] The commit message is inaccurate and incomplete.

--

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

This patch adds u64 casts to two u32 GP2 * NSEC_PER_USEC multiplications
in the iwlmld PTP code: one in iwl_mld_ptp_get_adj_time() and one in
iwl_mld_ptp_adjfine(). Without the casts, 32-bit kernels compute the
product in 32 bits, and it truncates for GP2 values above about 4.295
seconds.

> iwl_mld_ptp_gettime() and iwl_mld_ptp_get_crosstimestamp() in the same
> file already cast to u64 before multiplying.  Do the same here.

[Severity: Low]
This isn't a bug, but mld/ptp.c has no function called
iwl_mld_ptp_get_crosstimestamp(). Is this meant to be
iwl_mld_phc_get_crosstimestamp()?

The message also describes only the scale_time_gp2_ns line in
iwl_mld_ptp_get_adj_time(). It doesn't mention the second hunk, which
changes the gp2 * NSEC_PER_USEC argument in iwl_mld_ptp_adjfine().

Could the message cover both sites, and the user-visible effect, as the
mvm companion patch does?

> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/ptp.c b/drivers/net/wireless/intel/iwlwifi/mld/ptp.c
> index 1ac85d6ce1d79..41ba49f525845 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mld/ptp.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/ptp.c
> @@ -59,7 +59,8 @@ static void iwl_mld_ptp_update_new_read(struct iwl_mld *mld, u32 gp2)
>  u64 iwl_mld_ptp_get_adj_time(struct iwl_mld *mld, u64 base_time_ns)
>  {
>  	struct ptp_data *data = &mld->ptp_data;
> -	u64 scale_time_gp2_ns = mld->ptp_data.scale_update_gp2 * NSEC_PER_USEC;
> +	u64 scale_time_gp2_ns = (u64)mld->ptp_data.scale_update_gp2 *
> +				NSEC_PER_USEC;

[Severity: High]
Does one more caller of iwl_mld_ptp_get_adj_time() have the same
truncation? In mld/rx.c, the monitor mode PTP timestamp path in
iwl_mld_rx_fill_status() still does this:

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. On 32-bit, this
multiplication wraps before the result is widened to the u64 argument.

The mvm companion patch, "wifi: iwlwifi: mvm: Fix GP2 to nanoseconds
overflow on 32-bit", casts the matching gp2_on_air_rise site in
iwl_mvm_rx_fill_status().

The truncated value also reaches the shared wraparound tracking, because
iwl_mld_ptp_get_adj_time() does:

	iwl_mld_ptp_update_new_read(mld,
				    div64_u64(base_time_ns, NSEC_PER_USEC));

For example, take gp2_on_air_rise = 10,000,000 us. After the 32-bit wrap
the product is 1,410,065,408 ns, which divides back to 1,410,065 us.

If last_gp2 is around 10,000,000, the gap is larger than
IWL_PTP_WRAP_THRESHOLD_USEC. iwl_mld_ptp_update_new_read() then treats
the frame as a wraparound: it increments wrap_counter and overwrites
last_gp2.

Wouldn't that shift the PHC by 2^32 us for gettime64, adjfine and the
other PTP users, not just the radiotap mactime?

The path can be hit on 32-bit builds with CONFIG_IWLWIFI_DEBUGFS, monitor
mode on, and the rx_ts_ptp debugfs knob set.

Should the rx.c call get the same (u64) cast in this patch?

[ ... ]

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