Re: [PATCH net-next v8 1/3] net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 (4) · Low: 0

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] net: stmmac: TOCTOU race on STMMAC_FLAG_EXT_SNAPSHOT_EN check
- [High] net: stmmac: race condition in smtg_crosststamp leaves SMTG
  reads unprotected

Pre-existing issues:
- [Medium] intel_crosststamp() in
  drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c has the same…
- [Medium] smtg_crosststamp() (drivers/net/ethernet/stmicro/stmmac/dwmac-
  socfpga.c:310) tests STMMAC_FLAG_EXT_SNAPSHOT_EN before…
- [Medium] At this commit, the XGMAC/XLGMAC hwif entries use .hwtimestamp
  = &stmmac_ptp.
- [Medium] smtg_crosststamp() releases aux_ts_lock (dwmac-socfpga.c:391)
  before get_smtgtime() reads the system-counter half of the cross-…

--

net: stmmac: dwmac-socfpga: complete cross-timestamp on ATSNS

This patch changes smtg_crosststamp() so that it first waits for the
PTP_ACR_ATSFC FIFO clear to finish, then polls the ATSNS count in
XGMAC_TIMESTAMP_STATUS instead of the transient TSIS bit. It also holds
aux_ts_lock across the whole clear, trigger, poll and drain sequence.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 6d4bc1fe8f751..ee79b74bf21b6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -337,8 +337,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	acr_value = readl(ptpaddr + PTP_ACR);
>  	acr_value |= PTP_ACR_ATSFC;
>  	writel(acr_value, ptpaddr + PTP_ACR);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. In
smtg_crosststamp(), the STMMAC_FLAG_EXT_SNAPSHOT_EN check runs before
aux_ts_lock is taken, and it is not checked again once the lock is held:

	if (priv->plat->flags & STMMAC_FLAG_EXT_SNAPSHOT_EN)
		return -EBUSY;

	mutex_lock(&priv->aux_ts_lock);

stmmac_enable() sets that flag and programs PTP_ACR under aux_ts_lock:

	priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;

	/* Enable External snapshot trigger */
	acr_value |= PTP_ACR_ATSEN(rq->extts.index);
	acr_value |= PTP_ACR_ATSFC;

A PTP_CLK_REQ_EXTTS enable can finish between the unlocked check and
mutex_lock(). One way this happens is when both callers are blocked on
the mutex behind another crosststamp. smtg_crosststamp() then goes on to
do:

	acr_value &= ~PTP_ACR_MASK;

Does this clear the external ATSEN bit? And does the ATSFC write that
follows then flush any external snapshots already latched?

If so, EXT_SNAPSHOT_EN stays set and the EXTTS request has already
returned success, but external events are no longer latched and no error
is reported. Could the EXT_SNAPSHOT_EN check be moved under aux_ts_lock?

Also, at this commit smtg_crosststamp() does not set
STMMAC_FLAG_INT_SNAPSHOT_EN, so the shared timestamp_interrupt() could
drain the FIFO during this window. A later patch in the series, "net:
stmmac: dwxgmac2: add XGMAC timestamp interrupt support", fixes that
part. The unlocked EXT_SNAPSHOT_EN check is still there at the end of
the series.

> -	/* Release the mutex */
> -	mutex_unlock(&priv->aux_ts_lock);

[Severity: High]
This isn't introduced by this patch, and the window is narrower than
before. Still, the system counter half of the cross-timestamp is read
after aux_ts_lock has been dropped.

The SMTG block latches the ARM arch counter on the GPO0 rising edge
that smtg_crosststamp() generates. get_smtgtime() then fetches that
latched value with four separate mdiobus_read() calls, from
SMTG_TSC_WORD3 down to SMTG_TSC_WORD0. Each MDIO read is serialized
on its own, but nothing ties the four reads to the trigger that
produced the value.

Neither ptp_sys_offset_precise() nor get_device_system_crosststamp()
serializes calls into the getcrosststamp path. Two processes issuing
PTP_SYS_OFFSET_PRECISE on the same /dev/ptpX, for example phc2sys and
chronyd, can therefore run smtg_crosststamp() concurrently.

Suppose the first caller has dropped aux_ts_lock but has not finished
get_smtgtime(). The second caller can take aux_ts_lock, clear the FIFO
and toggle GPO0 in that window, which relatches the SMTG registers
under the first caller. The first caller then pairs its PTP device
time with the second capture, or with words torn across the two
captures, and still reports success.

That silently breaks the precision PTP_SYS_OFFSET_PRECISE is meant to
provide. Would it work to move this mutex_unlock() after
get_smtgtime()? Then the trigger, the FIFO drain and the SMTG read
would all happen under aux_ts_lock. Holding the mutex across the MDIO
accesses should be fine, since this path can already sleep. Is there
any path that takes aux_ts_lock with the MDIO bus lock held?

[Severity: Medium]
This isn't a bug introduced by this patch, but intel_crosststamp() in
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c still has the pattern
that this patch removes from the socfpga version:

dwmac-intel.c:intel_crosststamp() {
    ...
	acr_value |= PTP_ACR_ATSFC;
	writel(acr_value, ptpaddr + PTP_ACR);
	/* Release the mutex */
	mutex_unlock(&priv->aux_ts_lock);
    ...
}

It never waits for the ATSFC clear to complete. It also waits on the
transient TSIE bit through stmmac_cross_ts_isr():

	return (readl(priv->ioaddr + GMAC_INT_STATUS) & GMAC_INT_TSIE);

After that it reads ATSNS from GMAC_TIMESTAMP_STATUS and drains the aux
FIFO without holding aux_ts_lock:

	num_snapshot = (readl(ioaddr + GMAC_TIMESTAMP_STATUS) &
			GMAC_TIMESTAMP_ATSNS_MASK) >>
			GMAC_TIMESTAMP_ATSNS_SHIFT;

stmmac_enable() handles PTP_CLK_REQ_EXTTS by taking aux_ts_lock,
rewriting PTP_ACR and setting ATSFC. Can that flush or re-arm the FIFO
between the TSIE wait and the drain loop? A stale TSIE, or a FIFO clear
that is still in progress, could also produce a stale or missing device
sample.

If num_snapshot ends up as 0, system->cycles stays 0 and
get_device_system_crosststamp() returns -EINVAL. So the result is a
mismatched sample or a spurious error, not uninitialized data.

Would it make sense to apply the same fix to intel_crosststamp() in a
follow-up?

> +
> +	/* Wait for the FIFO clear, so the poll only sees snapshots from
> +	 * this trigger.
> +	 */

[ ... ]

> @@ -349,10 +359,14 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	gpio_value |= XGMAC_GPIO_GPO0;
>  	writel(gpio_value, ioaddr + XGMAC_GPIO_STATUS);
>  
> -	/* Poll for time sync operation done */
> -	ret = readl_poll_timeout(priv->ioaddr + XGMAC_INT_STATUS, v,
> -				 (v & XGMAC_INT_TSIS), 100, 10000);
> +	/* Poll ATSNS instead of the transient TSIS: the ATSNS count is
> +	 * the FIFO level and is not cleared by reading the register.
> +	 */
> +	ret = readl_poll_timeout(ioaddr + XGMAC_TIMESTAMP_STATUS, v,
> +				 FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK, v),
> +				 100, 10000);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. At
this commit, the shared timestamp ISR does not use this register on
XGMAC. The XGMAC and XLGMAC entries in hwif.c use:

		.hwtimestamp = &stmmac_ptp,

and timestamp_interrupt() in stmmac_hwtstamp.c reads the dwmac4 offset:

	ts_status = readl(priv->ioaddr + GMAC_TIMESTAMP_STATUS);

GMAC_TIMESTAMP_STATUS is 0xb20, while XGMAC_TIMESTAMP_STATUS is 0xd20
with ATSNS at GENMASK(29, 25). On XGMAC cores, does the EXTTS ISR read
an unrelated register both to clear the timestamp status and to get the
aux FIFO level? That would give wrong or missing EXTTS events.

This looks like it is fixed later in the series by "net: stmmac:
dwxgmac2: add XGMAC timestamp interrupt support". That patch adds
dwxgmac2_ptp, whose dwxgmac2_timestamp_interrupt() reads
XGMAC_TIMESTAMP_STATUS, and switches both XGMAC hwif entries to
&dwxgmac2_ptp.

[ ... ]

> @@ -375,6 +388,8 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  		read_unlock_irqrestore(&priv->ptp_lock, flags);
>  	}
>  
> +	mutex_unlock(&priv->aux_ts_lock);
> +
>  	get_smtgtime(priv->mii, SMTG_MDIO_ADDR, &smtg_time);
>  	system->cycles = smtg_time;

[Severity: Medium]
This isn't a bug introduced by this patch, but the system counter half
of the cross-timestamp is still read after aux_ts_lock is dropped. The
SMTG IP latches the ARM arch counter on the GPO0 trigger. get_smtgtime()
then reads it with four separate MDIO reads:

	ns = mdiobus_read(mii, smtg_addr, SMTG_TSC_WORD3);
	...
	ns |= mdiobus_read(mii, smtg_addr, SMTG_TSC_WORD0);

ptp_sys_offset_precise() does not take pincfg_mux, and
get_device_system_crosststamp() does not serialize get_time_fn. With two
concurrent PTP_SYS_OFFSET_PRECISE callers, can the second caller take
aux_ts_lock and toggle GPO0 again before or during the first caller's
MDIO reads?

In that case the first caller pairs its device timestamp with the second
capture, or with words torn across the two captures, and still returns
success.

Before this patch the mutex was dropped even before the GPO0 trigger, so
the window was wider then. Would moving mutex_unlock() after
get_smtgtime() close the remaining window?

[ ... ]

> @@ -337,8 +337,18 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	acr_value = readl(ptpaddr + PTP_ACR);
>  	acr_value |= PTP_ACR_ATSFC;
>  	writel(acr_value, ptpaddr + PTP_ACR);
> -	/* Release the mutex */
> -	mutex_unlock(&priv->aux_ts_lock);
> +
> +	/* Wait for the FIFO clear, so the poll only sees snapshots from
> +	 * this trigger.
> +	 */
> +	ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
> +				 !(acr_value & PTP_ACR_ATSFC), 10, 10000);
> +	if (ret) {
> +		mutex_unlock(&priv->aux_ts_lock);
> +		netdev_err(priv->dev, "%s: Failed to clear snapshot FIFO\n",
> +			   __func__);
> +		return ret;
> +	}
>  
>  	/* Trigger Internal snapshot signal. Create a rising edge by just toggle
>  	 * the GPO0 to low and back to high.

[Severity: High]
This isn't introduced by this patch, but there is still a hole at the
top of smtg_crosststamp(). The commit message says aux_ts_lock is now
held across the whole sequence so a concurrent PTP_CLK_REQ_EXTTS
request cannot interfere. However, the STMMAC_FLAG_EXT_SNAPSHOT_EN test
that returns -EBUSY runs before mutex_lock(&priv->aux_ts_lock), and it
is not repeated once the lock is held.

stmmac_enable() sets STMMAC_FLAG_EXT_SNAPSHOT_EN and writes
PTP_ACR_ATSEN(rq->extts.index) | PTP_ACR_ATSFC into PTP_ACR under
aux_ts_lock. So this interleaving looks possible:

  smtg_crosststamp()           stmmac_enable(EXTTS, on)
  flag check passes
                               mutex_lock(aux_ts_lock)
                               set EXT_SNAPSHOT_EN
                               set ATSEN(n) | ATSFC in PTP_ACR
                               mutex_unlock(aux_ts_lock)
                               return 0
  mutex_lock(aux_ts_lock)
  acr_value &= ~PTP_ACR_MASK
  set ATSEN(int_snapshot_num)
  set ATSFC

Doesn't that drop the external ATSEN bit? And doesn't the ATSFC write
that follows flush any external snapshots already latched? Nothing
restores the external trigger afterwards. EXT_SNAPSHOT_EN stays set,
so later crosststamp calls return -EBUSY. Meanwhile the EXTTS request,
which already returned success, silently stops producing events.

Could the EXT_SNAPSHOT_EN check be moved after mutex_lock(), with an
unlock on the -EBUSY path? Then the check and the PTP_ACR update would
be done under the same lock.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013033207.1684328-1-zxyan0222%40gmail.com