netdev - Re: [PATCH iwl-net v1] igc: fix race condition in TX timestamp read for register 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0fc877a5-4b35-4802-9cda-e4eca561c5d1@linux.dev>
Date: Thu, 18 Sep 2025 21:47:33 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Chwee-Lin Choong <chwee.lin.choong@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S . Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Richard Cochran <richardcochran@...il.com>,
 Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Avi Shalev <avi.shalev@...el.com>,
 Song Yoong Siang <yoong.siang.song@...el.com>
Subject: Re: [PATCH iwl-net v1] igc: fix race condition in TX timestamp read
 for register 0

On 18/09/2025 19:38, Chwee-Lin Choong wrote:
> The current HW bug workaround checks the TXTT_0 ready bit first,
> then reads LOW -> HIGH -> LOW from register 0 to detect if a
> timestamp was captured.
> 
> This sequence has a race: if a new timestamp is latched after
> reading the TXTT mask but before the first LOW read, both old
> and new timestamp match, causing the driver to drop a valid
> timestamp.
> 
> Fix by reading the LOW register first, then the TXTT mask,
> so a newly latched timestamp will always be detected.
> 
> This fix also prevents TX unit hangs observed under heavy
> timestamping load.
> 
> Fixes: c789ad7cbebc ("igc: Work around HW bug causing missing timestamps")
> Suggested-by: Avi Shalev <avi.shalev@...el.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> Signed-off-by: Chwee-Lin Choong <chwee.lin.choong@...el.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_ptp.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 

[...]

>   		 * timestamp was captured, we can read the "high"
>   		 * register again.
>   		 */

This comment begins with 'read the "high" register (to latch a new 
timestamp)' ...

> -		u32 txstmpl_old, txstmpl_new;
> +		u32 txstmpl_new;
>   
> -		txstmpl_old = rd32(IGC_TXSTMPL);
>   		rd32(IGC_TXSTMPH);
>   		txstmpl_new = rd32(IGC_TXSTMPL);

and a couple of lines later in this function you have

		regval = txstmpl_new;
		regval |= (u64)rd32(IGC_TXSTMPH) << 32;

According to the comment above, the value in the register will be
latched after reading IGC_TXSTMPH. As there will be no read of "low"
part of the register, it will stay latched with old value until the
next call to the same function. Could it be the reason of unit hangs?

It looks like the value of previous read of IGC_TXSTMPH should be stored
and used to construct new timestamp, right?