Re: [PATCH net-next 3/7] ptp: netc: export netc_timer_get_current_time() for cross-driver use

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) race condition in netc_timer_get_current_time() leads to Use-After-Free and invalid MMIO access during concurrent unbind.
- [High] Premature publishing of driver data in netc_timer_pci_probe() causes netc_timer_get_current_time() to access uninitialized locks and hardware registers.
- [Low] Build/linker error due to using IS_ENABLED() instead of IS_REACHABLE() for an exported cross-driver symbol.
--

--- Patch [3]: [PATCH net-next 3/7] ptp: netc: export netc_timer_get_current_time() for cross-driver use ---
commit 397844577b2034cd8a6a870bc6609d9217d7fc38
Author: Wei Fang <[email protected]>

ptp: netc: export netc_timer_get_current_time() for cross-driver use

This commit adds and exports netc_timer_get_current_time() to allow the NETC
Switch driver to read the current PTP time from the NETC Timer. It also
provides a stub declaration when the timer configuration is disabled.

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> index 072809ea7922c..60e64542314b1 100644
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c

[ ... ]

> @@ -165,6 +165,23 @@ static u64 netc_timer_cur_time_read(struct netc_timer *priv)
>  	return netc_timer_rd64(priv, NETC_TMR_CUR_TIME_L);
>  }
>  
> +u64 netc_timer_get_current_time(struct pci_dev *pdev)
> +{
> +	struct netc_timer *priv = pci_get_drvdata(pdev);

[Severity: High]
Does this function assume the driver data is fully initialized if priv is
not NULL?

In netc_timer_pci_probe(), pci_set_drvdata() is called before the spinlock
is initialized in netc_timer_probe(). If a concurrent caller fetches the
driver data here while the timer is still probing, could it attempt to
acquire an uninitialized spinlock or read from unconfigured hardware?

> +	unsigned long flags;
> +	u64 cur_time;
> +
> +	if (!priv)
> +		return 0;
> +

[Severity: Critical]
Is this lockless check sufficient to prevent a use-after-free if the timer
driver is unbound concurrently?

If a concurrent thread unbinds the timer driver via sysfs right after this
check passes, the driver's remove() callback could execute and unmap the
MMIO region. The device core's devres cleanup would then free the priv
pointer before clearing the driver data.

Does this mean the caller could then attempt to acquire a freed spinlock
and read from unmapped MMIO memory?

> +	spin_lock_irqsave(&priv->lock, flags);
> +	cur_time = netc_timer_cur_time_read(priv);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return cur_time;
> +}
> +EXPORT_SYMBOL_GPL(netc_timer_get_current_time);

[ ... ]

> diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
> index 5b8ff528d3699..91441cb35bc28 100644
> --- a/include/linux/fsl/netc_global.h
> +++ b/include/linux/fsl/netc_global.h
> @@ -22,4 +22,13 @@ static inline u64 netc_read64(void __iomem *reg)
>  	return ioread64(reg);
>  }
>  
> +#if IS_ENABLED(CONFIG_PTP_NETC_V4_TIMER)

[Severity: Low]
Will this cause a linker error if the calling driver is built-in (=y) but
the timer driver is built as a module (=m)?

Since IS_ENABLED() evaluates to true when the config is a module, the
fallback stub won't be provided. The built-in caller would then be unable
to resolve the modular symbol during linking.

Should this use IS_REACHABLE() instead?

> +u64 netc_timer_get_current_time(struct pci_dev *timer_dev);
> +#else
> +static inline u64 netc_timer_get_current_time(struct pci_dev *timer_dev)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3