| 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
| ||
|
Message-ID: <87zf81fx9y.fsf@intel.com>
Date: Mon, 01 Dec 2025 18:11:21 -0800
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
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>
Cc: yipeng.chai@....com, alexander.deucher@....com,
intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Aleksandr Loktionov
<aleksandr.loktionov@...el.com>, Avi Shalev <avi.shalev@...el.com>, Song
Yoong Siang <yoong.siang.song@...el.com>, Chwee-Lin Choong
<chwee.lin.choong@...el.com>
Subject: Re: [PATCH iwl-net v4] igc: fix race condition in TX timestamp read
for register 0
Hi,
Chwee-Lin Choong <chwee.lin.choong@...el.com> writes:
> The current HW bug workaround checks the TXTT_0 ready bit first,
> then reads TXSTMPL_0 twice (before and after reading TXSTMPH_0)
> to detect whether a new timestamp was captured by timestamp
> register 0 during the workaround.
>
> This sequence has a race: if a new timestamp is captured after
> checking the TXTT_0 bit but before the first TXSTMPL_0 read, the
> detection fails because both the “old” and “new” values come from
> the same timestamp.
>
> Fix by reading TXSTMPL_0 first to establish a baseline, then
> checking the TXTT_0 bit. This ensures any timestamp captured
> during the race window will be detected.
>
> Old sequence:
> 1. Check TXTT_0 ready bit
> 2. Read TXSTMPL_0 (baseline)
> 3. Read TXSTMPH_0 (interrupt workaround)
> 4. Read TXSTMPL_0 (detect changes vs baseline)
>
> New sequence:
> 1. Read TXSTMPL_0 (baseline)
> 2. Check TXTT_0 ready bit
> 3. Read TXSTMPH_0 (interrupt workaround)
> 4. Read TXSTMPL_0 (detect changes vs baseline)
>
> Fixes: c789ad7cbebc ("igc: Work around HW bug causing missing timestamps")
> Suggested-by: Avi Shalev <avi.shalev@...el.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> Co-developed-by: Song Yoong Siang <yoong.siang.song@...el.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> Signed-off-by: Chwee-Lin Choong <chwee.lin.choong@...el.com>
Patch looks good, my only concern is this report:
https://lore.kernel.org/intel-wired-lan/AS1PR10MB5675DBFE7CE5F2A9336ABFA4EBEAA@AS1PR10MB5675.EURPRD10.PROD.OUTLOOK.COM/
It's not clear to me how/why the different buffer utilization is
affecting this, but at least seems worth some investigation and
reporting back in that thread.
> ---
> v1: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20250918183811.31270-1-chwee.lin.choong@intel.com/
> v2: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20251127134927.2133-1-chwee.lin.choong@intel.com/
> v3: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20251127151137.2883-1-chwee.lin.choong@intel.com/
>
> changelog:
> v1 -> v2
> - Added detailed comments explaining the hardware bug workaround and race
> detection mechanism
> v2 -> v3
> - Removed extra export file added by mistake
> v3 -> v4
> - Added co-developer
> ---
> drivers/net/ethernet/intel/igc/igc_ptp.c | 43 ++++++++++++++----------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index b7b46d863bee..7aae83c108fd 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -774,36 +774,43 @@ static void igc_ptp_tx_reg_to_stamp(struct igc_adapter *adapter,
> static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
> {
> struct igc_hw *hw = &adapter->hw;
> + u32 txstmpl_old;
> u64 regval;
> u32 mask;
> int i;
>
> + /* Establish baseline of TXSTMPL_0 before checking TXTT_0.
> + * This baseline is used to detect if a new timestamp arrives in
> + * register 0 during the hardware bug workaround below.
> + */
> + txstmpl_old = rd32(IGC_TXSTMPL);
> +
> mask = rd32(IGC_TSYNCTXCTL) & IGC_TSYNCTXCTL_TXTT_ANY;
> if (mask & IGC_TSYNCTXCTL_TXTT_0) {
> regval = rd32(IGC_TXSTMPL);
> regval |= (u64)rd32(IGC_TXSTMPH) << 32;
> } else {
> - /* There's a bug in the hardware that could cause
> - * missing interrupts for TX timestamping. The issue
> - * is that for new interrupts to be triggered, the
> - * IGC_TXSTMPH_0 register must be read.
> + /* TXTT_0 not set - register 0 has no new timestamp initially.
> + *
> + * Hardware bug: Future timestamp interrupts won't fire unless
> + * TXSTMPH_0 is read, even if the timestamp was captured in
> + * registers 1-3.
> *
> - * To avoid discarding a valid timestamp that just
> - * happened at the "wrong" time, we need to confirm
> - * that there was no timestamp captured, we do that by
> - * assuming that no two timestamps in sequence have
> - * the same nanosecond value.
> + * Workaround: Read TXSTMPH_0 here to enable future interrupts.
> + * However, this read clears TXTT_0. If a timestamp arrives in
> + * register 0 after checking TXTT_0 but before this read, it
> + * would be lost.
> *
> - * So, we read the "low" register, read the "high"
> - * register (to latch a new timestamp) and read the
> - * "low" register again, if "old" and "new" versions
> - * of the "low" register are different, a valid
> - * timestamp was captured, we can read the "high"
> - * register again.
> + * To detect this race: We saved a baseline read of TXSTMPL_0
> + * before TXTT_0 check. After performing the workaround read of
> + * TXSTMPH_0, we read TXSTMPL_0 again. Since consecutive
> + * timestamps never share the same nanosecond value, a change
> + * between the baseline and new TXSTMPL_0 indicates a timestamp
> + * arrived during the race window. If so, read the complete
> + * timestamp.
> */
> - u32 txstmpl_old, txstmpl_new;
> + u32 txstmpl_new;
>
> - txstmpl_old = rd32(IGC_TXSTMPL);
> rd32(IGC_TXSTMPH);
> txstmpl_new = rd32(IGC_TXSTMPL);
>
> @@ -818,7 +825,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>
> done:
> /* Now that the problematic first register was handled, we can
> - * use retrieve the timestamps from the other registers
> + * retrieve the timestamps from the other registers
> * (starting from '1') with less complications.
> */
> for (i = 1; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
> --
> 2.43.0
>
--
Vinicius
Powered by blists - more mailing lists