Re: [PATCH net v4 3/3] sfc: ptp: avoid racing PPS timestamp updates

[netdev-bot+sashiko]

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

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

- [Medium] This patch removes the shared ptp->host_time_pps race from
  drivers/net/ethernet/sfc/ptp.c, but the same race is still present in…

--

sfc: ptp: avoid racing PPS timestamp updates

This patch removes the shared host_time_pps field from struct
efx_ptp_data in sfc/ptp.c. efx_ptp_synchronize() now returns the PPS
timestamp through storage owned by the caller. The PPS worker therefore
no longer copies a field that a concurrent SIOCSHWTSTAMP-driven
synchronization could be rewriting.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index e26269fae38d6..37511f234d76d 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -1500,14 +1504,15 @@ static void efx_ptp_pps_worker(struct work_struct *work)
>  {
>  	struct efx_ptp_data *ptp =
>  		container_of(work, struct efx_ptp_data, pps_work);
> +	struct pps_event_time host_time_pps = {};
>  	struct efx_nic *efx = ptp->efx;
>  	struct ptp_clock_event ptp_evt;
>  
> -	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
> +	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS, &host_time_pps))
>  		return;
>  
>  	ptp_evt.type = PTP_CLOCK_PPSUSR;
> -	ptp_evt.pps_times = ptp->host_time_pps;
> +	ptp_evt.pps_times = host_time_pps;
>  	ptp_clock_event(ptp->phc_clock, &ptp_evt);
>  }

[Severity: Medium]
This fixes the race in sfc/ptp.c. Is the same race still present in the
Siena copy of this code, drivers/net/ethernet/sfc/siena/ptp.c? That file
also descends from the Fixes target 7c236c43b838, and this patch doesn't
touch it.

In siena/ptp.c, efx_ptp_process_times() still writes the shared field:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_process_times() {
    ...
	/* Set PPS timestamp to match NIC top of second */
	ptp->host_time_pps = *last_time;
	pps_sub_ts(&ptp->host_time_pps, delta);
    ...
}

The Siena efx_ptp_pps_worker() copies it after synchronization returns,
with no lock held:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_pps_worker() {
    ...
	ptp_evt.type = PTP_CLOCK_PPSUSR;
	ptp_evt.pps_times = ptp->host_time_pps;
	ptp_clock_event(ptp->phc_clock, &ptp_evt);
}

Meanwhile, the SIOCSHWTSTAMP path can run another synchronization:

efx_siena_ptp_change_mode()
    efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
        efx_ptp_process_times()
            ptp->host_time_pps = *last_time;

Can this give the partially updated PPS timestamp that the commit message
describes, but for Siena NICs?

Siena also looks more exposed than sfc was before this series. The Siena
efx_ptp_synchronize() takes no phc_lock or any other mutex. The
WRITE_ONCE(*start, 0) handshake runs before efx_siena_mcdi_rpc_start(),
and the ptp->timeset[] processing runs after efx_siena_mcdi_rpc_finish().
Could two concurrent synchronizations clobber each other's state there
too?

The earlier patch in this series, "sfc: ptp: serialize host and MC
synchronization transactions", seems to have handled that only for sfc.

Should the same caller-owned out-parameter change, along with the
serialization, also be applied to siena/ptp.c? If not, could the commit
message say why Siena is left out?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026123023.3566549-1-alex.austin%40amd.com