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

[Jordan Rhee]

On Sun, Mar 29, 2026 at 2:20 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> 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.

Ack, will fix.

>
> >       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?

Ack, will fix.

> --
> pw-bot: cr