From: Ankit Garg <nktgrg@google.com>
Add a mutex to protect the shared DMA buffer that receives NIC
timestamp reports. The NIC timestamp will be read from two different
threads: the periodic worker and upcoming `gettimex64`.
Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Ankit Garg <nktgrg@google.com>
Signed-off-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
drivers/net/ethernet/google/gve/gve.h | 6 +----
drivers/net/ethernet/google/gve/gve_ptp.c | 32 +++++++++++++++--------
2 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 1d66d3834f7e..7b8f78bd1968 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -923,6 +923,7 @@ struct gve_priv {
bool nic_timestamp_supported;
struct gve_ptp *ptp;
struct kernel_hwtstamp_config ts_config;
+ struct mutex nic_ts_read_lock; /* Protects nic_ts_report */
struct gve_nic_ts_report *nic_ts_report;
dma_addr_t nic_ts_report_bus;
u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
@@ -1321,14 +1322,9 @@ int gve_flow_rules_reset(struct gve_priv *priv);
int gve_init_rss_config(struct gve_priv *priv, u16 num_queues);
/* PTP and timestamping */
#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
-int gve_clock_nic_ts_read(struct gve_priv *priv);
int gve_init_clock(struct gve_priv *priv);
void gve_teardown_clock(struct gve_priv *priv);
#else /* CONFIG_PTP_1588_CLOCK */
-static inline int gve_clock_nic_ts_read(struct gve_priv *priv)
-{
- return -EOPNOTSUPP;
-}
static inline int gve_init_clock(struct gve_priv *priv)
{
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
@@ -11,19 +11,20 @@
#define GVE_NIC_TS_SYNC_INTERVAL_MS 250
/* Read the nic timestamp from hardware via the admin queue. */
-int gve_clock_nic_ts_read(struct gve_priv *priv)
+static int gve_clock_nic_ts_read(struct gve_priv *priv, u64 *nic_raw)
{
- u64 nic_raw;
int err;
+ mutex_lock(&priv->nic_ts_read_lock);
err = gve_adminq_report_nic_ts(priv, priv->nic_ts_report_bus);
if (err)
- return err;
+ goto out;
- nic_raw = be64_to_cpu(priv->nic_ts_report->nic_timestamp);
- WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
+ *nic_raw = be64_to_cpu(priv->nic_ts_report->nic_timestamp);
- return 0;
+out:
+ mutex_unlock(&priv->nic_ts_read_lock);
+ return err;
}
static int gve_ptp_gettimex64(struct ptp_clock_info *info,
@@ -43,15 +44,19 @@ static long gve_ptp_do_aux_work(struct ptp_clock_info *info)
{
const struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
struct gve_priv *priv = ptp->priv;
+ u64 nic_raw;
int err;
if (gve_get_reset_in_progress(priv) || !gve_get_admin_queue_ok(priv))
goto out;
- err = gve_clock_nic_ts_read(priv);
- if (err && net_ratelimit())
- dev_err(&priv->pdev->dev,
- "%s read err %d\n", __func__, err);
+ err = gve_clock_nic_ts_read(priv, &nic_raw);
+ if (err) {
+ dev_err_ratelimited(&priv->pdev->dev, "%s read err %d\n",
+ __func__, err);
+ goto out;
+ }
+ WRITE_ONCE(priv->last_sync_nic_counter, nic_raw);
out:
return msecs_to_jiffies(GVE_NIC_TS_SYNC_INTERVAL_MS);
@@ -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);
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);
@@ -150,6 +159,7 @@ void gve_teardown_clock(struct gve_priv *priv)
gve_ptp_release(priv);
if (priv->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);
--
2.53.0.1018.g2bb0e51243-googThis 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
On Sun, Mar 29, 2026 at 2:20 PM Jakub Kicinski <kuba@kernel.org> 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
© 2016 - 2026 Red Hat, Inc.