Re: [PATCH net-next v2 3/3] gve: implement PTP gettimex64

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: gve: implement PTP gettimex64

This commit implements the PTP gettimex64 function to enable chrony and
phc2sys to synchronize the system clock to the NIC clock. It reads the
NIC timestamp along with pre and post system cycle counters to minimize
the uncertainty window.

> diff --git a/drivers/net/ethernet/google/gve/gve_ptp.c b/drivers/net/ethernet/google/gve/gve_ptp.c
> index 140b8fbce4f4..bea794541f30 100644
> --- a/drivers/net/ethernet/google/gve/gve_ptp.c
> +++ b/drivers/net/ethernet/google/gve/gve_ptp.c
> @@ -10,28 +10,203 @@

[ ... ]

> +static int gve_adminq_read_timestamp(struct gve_priv *priv,
> +				     cycles_t *pre_cycles,
> +				     cycles_t *post_cycles)
> +{
> +	unsigned long delay_us = 1000;
> +	int retry_count = 0;
> +	int err;
> +
> +	lockdep_assert_held(&priv->nic_ts_read_lock);
> +
> +	do {
> +		*pre_cycles = get_cycles();
> +		err = gve_adminq_report_nic_ts(priv, priv->nic_ts_report_bus);
> +
> +		/* Ensure cycle counter is sampled after AdminQ cmd returns */
> +		rmb();
> +		*post_cycles = get_cycles();

Are execution barriers needed around get_cycles() here? rmb() orders
memory accesses, but the CPU might still speculatively execute the cycle
counter reads out of order with respect to the AdminQ commands.

Could this speculative reordering cause the strict bounds validation in
gve_ptp_gettimex64() to fail spuriously?

[ ... ]

>  static int gve_ptp_gettimex64(struct ptp_clock_info *info,
>  			      struct timespec64 *ts,
>  			      struct ptp_system_timestamp *sts)
>  {
> -	return -EOPNOTSUPP;
> +	struct gve_ptp *ptp = container_of(info, struct gve_ptp, info);
> +	struct gve_sysclock_sample sysclock = {0};
> +	struct gve_priv *priv = ptp->priv;
> +	struct system_time_snapshot snap;
> +	u64 nic_ts;
> +	int err;
> +
> +	/* Take system clock snapshot before sampling cycle counters */
> +	if (sts)
> +		ktime_get_snapshot(&snap);
> +
> +	err = gve_clock_nic_ts_read(priv, &nic_ts, &sysclock);

Could the system time snapshot become stale here? 

If priv->nic_ts_read_lock is contended, or if gve_adminq_read_timestamp()
retries and sleeps, the snapshot might be taken significantly before the
actual hardware read.

Would it be more accurate to take the snapshot immediately before reading the
hardware, inside the mutex?

> +	if (err)
> +		return err;
> +
> +	if (sts) {
> +		/* Reject samples with out of order system clock values */
> +		if (!(sysclock.host_pre_cycles <= sysclock.nic_pre_cycles &&
> +		      sysclock.nic_pre_cycles  <= sysclock.nic_post_cycles &&
> +		      sysclock.nic_post_cycles <= sysclock.host_post_cycles)) {
> +			dev_err_ratelimited(&priv->pdev->dev,
> +					    "AdminQ system clock cycle counts out of order. Expecting %llu <= %llu <= %llu <= %llu\n",
> +					    (u64)sysclock.host_pre_cycles,
> +					    sysclock.nic_pre_cycles,
> +					    sysclock.nic_post_cycles,
> +					    (u64)sysclock.host_post_cycles);
> +			return -EBADMSG;

How does this behave on architectures where get_cycles() is not implemented
and returns 0, or with older firmware that leaves nic_pre_cycles and
nic_post_cycles as 0?

It looks like the bounds check would unconditionally fail and return -EBADMSG.
Since userspace tools like chrony usually expect -EOPNOTSUPP for unsupported
extended ioctls to fall back to basic PTP ioctls, will returning -EBADMSG
cause a hard failure for clock synchronization?

[ ... ]

> @@ -132,7 +307,7 @@ int gve_init_clock(struct gve_priv *priv)
>  		goto release_ptp;
>  	}
>  	mutex_init(&priv->nic_ts_read_lock);
> -	err = gve_clock_nic_ts_read(priv, &nic_raw);
> +	err = gve_clock_nic_ts_read(priv, &nic_raw, NULL);
>  	if (err) {
>  		dev_err(&priv->pdev->dev, "failed to read NIC clock %d\n", err);
>  		goto release_nic_ts_report;

Is there a race condition exposing the PTP device to userspace before its
resources are fully initialized?

Looking at gve_init_clock(), gve_ptp_init() is called before allocating
nic_ts_report and initializing nic_ts_read_lock:

    err = gve_ptp_init(priv);
    if (err)
        return err;

    priv->nic_ts_report = dma_alloc_coherent(...);
    ...
    mutex_init(&priv->nic_ts_read_lock);

If a concurrent userspace process immediately invokes the
PTP_SYS_OFFSET_EXTENDED ioctl after gve_ptp_init() registers the /dev/ptpX
device, could it call gve_ptp_gettimex64() and attempt to lock the
uninitialized mutex or dereference the NULL nic_ts_report pointer?

Additionally, in the error path for gve_init_clock():

    release_nic_ts_report:
        mutex_destroy(&priv->nic_ts_read_lock);
        dma_free_coherent(...);
        priv->nic_ts_report = NULL;
    release_ptp:
        gve_ptp_release(priv);

Could destroying the mutex and freeing the memory before gve_ptp_release()
create a use-after-free window if an ioctl is currently running?