Re: [PATCH net-next v2 2/3] gve: make nic clock reads thread safe

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
gve: make nic clock reads thread safe

This commit adds a mutex to protect the shared DMA buffer used for receiving
NIC timestamp reports. The lock is needed to synchronize the periodic worker
and the upcoming gettimex64 ioctl.

> diff --git a/drivers/net/ethernet/google/gve/gve_ptp.c b/drivers/net/ethernet/google/gve/gve_ptp.c
> index 06b1cf4a5efc..140b8fbce4f4 100644
> --- a/drivers/net/ethernet/google/gve/gve_ptp.c
> +++ b/drivers/net/ethernet/google/gve/gve_ptp.c

[ ... ]

> @@ -109,6 +114,7 @@ static void gve_ptp_release(struct gve_priv *priv)
>  
>  int gve_init_clock(struct gve_priv *priv)
>  {
> +	u64 nic_raw;
>  	int err;
>  
>  	err = gve_ptp_init(priv);

[ ... ]

> @@ -125,17 +131,20 @@ int gve_init_clock(struct gve_priv *priv)
>  		err = -ENOMEM;
>  		goto release_ptp;
>  	}
> -	err = gve_clock_nic_ts_read(priv);
> +	mutex_init(&priv->nic_ts_read_lock);
> +	err = gve_clock_nic_ts_read(priv, &nic_raw);

Could this lead to a race condition with userspace?

If gve_ptp_init() registers the PTP clock via ptp_clock_register(), the
device is exposed to userspace immediately. If a userspace process opens the
device and calls the gettimex64 ioctl before the mutex and DMA buffer are
initialized here, it could cause a NULL pointer dereference or operate on an
uninitialized mutex.

>  	if (err) {
>  		dev_err(&priv->pdev->dev, "failed to read NIC clock %d\n", err);
>  		goto release_nic_ts_report;
>  	}
> +	WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
>  	ptp_schedule_worker(priv->ptp->clock,
>  			    msecs_to_jiffies(GVE_NIC_TS_SYNC_INTERVAL_MS));
>  
>  	return 0;
>  
>  release_nic_ts_report:
> +	mutex_destroy(&priv->nic_ts_read_lock);
>  	dma_free_coherent(&priv->pdev->dev,
>  			  sizeof(struct gve_nic_ts_report),
>  			  priv->nic_ts_report, priv->nic_ts_report_bus);

Is it safe to destroy the mutex and free the DMA buffer before calling
gve_ptp_release() here?

If the PTP clock is already registered and exposed to userspace by
gve_ptp_init(), this error path frees resources while the clock is still
active. A concurrent userspace ioctl could access the destroyed mutex or
freed buffer before gve_ptp_release() unregisters the clock.

Should all resource allocation and initialization strictly precede
ptp_clock_register(), and the error path unregister the clock before tearing
down the resources?
-- 
pw-bot: cr