Re: [PATCH net] ptp: vclock: reject failed physical clock samples

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Failed PHC reads can still put virtual time off by a 32-bit
  wrap.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  rollback path inherits it.

--

ptp: vclock: reject failed physical clock samples

With this patch, the virtual clock samples the parent PHC before it touches
the timecounter. Only successful samples are cached for the cyclecounter
read callback, and errors from adjfine, gettime and settime are passed back
to the caller. The patch also moves the initial timecounter_init() ahead of
ptp_clock_register() and adds a rollback in n_vclocks_store() for when
registration fails partway through.

> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index dc398c6b75284..9c25d897be196 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c

[ ... ]

> @@ -257,6 +257,12 @@ static ssize_t n_vclocks_store(struct device *dev,
>  	mutex_unlock(&ptp->n_vclocks_mux);
>  
>  	return count;
> +err_register:
> +	num = i;
> +	if (num)
> +		device_for_each_child_reverse(dev, &num, unregister_vclock);

[Severity: High]
This isn't a bug introduced by this patch, but the new rollback path
inherits an existing race with parent clock teardown.

In ptp_clock_unregister(), ptp_vclock_in_use() takes n_vclocks_mux and
releases it again before the children are walked:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
	if (ptp_vclock_in_use(ptp)) {
		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
	}
	...
	posix_clock_unregister(&ptp->clock);
	...
}

The n_vclocks sysfs attribute stays in place until posix_clock_unregister()
runs. During that window, n_vclocks_store() can reach unregister_vclock()
in two ways: through the existing deletion branch, or through this new
err_register path. Either iterator can pick the same child as the one in
ptp_clock_unregister().

The klist iterator holds a reference on the child struct device. It does
not hold one on the separately allocated struct ptp_vclock, and
ptp_vclock_unregister() frees that directly:

drivers/ptp/ptp_vclock.c:ptp_vclock_unregister() {
	...
	ptp_vclock_hash_del(vclock);

	ptp_clock_unregister(vclock->clock);
	kfree(vclock);
}

If both callbacks hit the same child, can the second unregister_vclock()
run ptp_vclock_unregister() on freed memory? That would mean a double
ptp_clock_unregister() and a double kfree() of the vclock.

PHC reads may fail while the hardware is being removed. That makes a
failed ptp_vclock_register() during teardown plausible, so this rollback
adds another way into the race.

> +	for (num = 0; num < i; num++)
> +		ptp->vclock_index[ptp->n_vclocks + num] = -1;
>  out:
>  	mutex_unlock(&ptp->n_vclocks_mux);
>  	return err;
> diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
> index 84cb527f59ccc..6378e9a8cd805 100644
> --- a/drivers/ptp/ptp_vclock.c
> +++ b/drivers/ptp/ptp_vclock.c
> @@ -42,21 +42,41 @@ static void ptp_vclock_hash_del(struct ptp_vclock *vclock)
>  	synchronize_srcu(&vclock_srcu);
>  }
>  
> +/* Sample before changing the timecounter. Its read callback cannot return an
> + * error, so passing a failed PHC read through it would fabricate a wraparound.
> + * The caller holds vclock->lock, or has not published the clock yet.
> + */
> +static int ptp_vclock_sample(struct ptp_vclock *vclock)
> +{
> +	struct ptp_clock *ptp = vclock->pclock;
> +	struct timespec64 ts;
> +	int err;
> +
> +	err = ptp->info->getcycles64(ptp->info, &ts);
> +	if (!err)
> +		vclock->cycles = timespec64_to_ns(&ts);
> +	return err;
> +}

[ ... ]

> @@ -76,14 +96,18 @@ static int ptp_vclock_gettime(struct ptp_clock_info *ptp,
>  {
>  	struct ptp_vclock *vclock = info_to_vclock(ptp);
>  	u64 ns;
> +	int err;
>  
>  	if (mutex_lock_interruptible(&vclock->lock))
>  		return -EINTR;
> -	ns = timecounter_read(&vclock->tc);
> +	err = ptp_vclock_sample(vclock);
> +	if (!err) {
> +		ns = timecounter_read(&vclock->tc);
> +		*ts = ns_to_timespec64(ns);
> +	}
>  	mutex_unlock(&vclock->lock);
> -	*ts = ns_to_timespec64(ns);
>  
> -	return 0;
> +	return err;
>  }

[Severity: High]
Can a failed refresh still put virtual time off by a 32-bit wrap?

ptp_vclock_refresh() is not changed and still ignores the error that
ptp_vclock_gettime() now returns:

drivers/ptp/ptp_vclock.c:ptp_vclock_refresh() {
	...
	ptp_vclock_gettime(&vclock->info, &ts);

	return PTP_VCLOCK_REFRESH_INTERVAL;
}

So when ptp_vclock_sample() fails, tc->cycle_last is left alone, and the
next try comes a full 2 seconds later. ptp_vclock_cc uses
CYCLECOUNTER_MASK(32) on nanosecond values. That gives a wrap horizon of
about 4.29s and a forward window in timecounter_cyc2time() of about 2.147s.

With one failed refresh, the last good sample is about 4s old by the next
refresh. Once it is more than about 2.147s old, timecounter_cyc2time()
treats current timestamps as past ones:

include/linux/timecounter.h:timecounter_cyc2time() {
	...
	if (unlikely(delta > cc->mask / 2)) {
		delta = (tc->cycle_last - cycle_tstamp) & cc->mask;
		nsec -= cc_cyc2ns_backwards(cc, delta, frac);
	...
}

Would ptp_convert_timestamp(), ptp_vclock_gettimex() and
ptp_vclock_getcrosststamp() then return values about 4.29s early?

When the parent has getcyclesx64, ptp_vclock_gettimex() never advances
cycle_last. Only refresh, adjfine and settime can recover it, and all three
sample the same failing PHC.

With two failed refreshes in a row, the gap is at least 6s. The next
successful timecounter_read() then goes through timecounter_read_delta():

	cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask;

This looks like it drops a whole 2^32 ns from virtual time, and the loss is
permanent. One failure plus more than about 295ms of worker delay would
have the same effect.

In this case the masked delta now always comes out one wrap short. The old
path, which consumed a zero sample and then the real value, gave the
correct elapsed time for some counter phases.

The initial sample in ptp_vclock_register() is now taken before
ptp_clock_register(). The time spent registering therefore adds to the age
of cycle_last before the first refresh runs.

The commit message says a failed sample "can add an extra 32-bit wrap to
virtual time". Is that still reachable through failed refreshes?

Would it make sense for ptp_vclock_refresh() to retry sooner on failure?
Another option would be to handle gaps longer than the mask using the full
64-bit ns sample.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-ptp-vclock-sampling-v1-1-c11bc16691ae%40gmail.com