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

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

[James Hilliard]

The cyclecounter read callback cannot report errors. Passing an
unsuccessful PHC read through it consumes an invalid sample, and a zero
sample followed by the real counter can add an extra 32-bit wrap to
virtual time.

Read the parent clock before updating or initializing the timecounter,
propagate failures from clock operations, and leave its state unchanged
on failure. Cache only the successful sample for the infallible
cyclecounter callback. Initialize this state before publishing a newly
registered virtual clock.

The initial sample can also fail partway through a sysfs request to create
multiple virtual clocks. Unregister any clocks created by that request
and clear their index entries, preserving the previously installed clocks
and count. Otherwise the failed request leaves registered children that
are not included in n_vclocks. This cleanup also handles existing
allocation and registration failure paths.

Fixes: 5d43f951b1ac ("ptp: add ptp virtual clock driver framework")
Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/ptp/ptp_private.h |  1 +
 drivers/ptp/ptp_sysfs.c   |  8 ++++++-
 drivers/ptp/ptp_vclock.c  | 56 +++++++++++++++++++++++++++++++++++------------
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index db4039d642b4..4ff22adda652 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -75,6 +75,7 @@ struct ptp_clock {
 #define dw_to_vclock(d) container_of((d), struct ptp_vclock, refresh_work)
 
 struct ptp_vclock {
+	u64 cycles;
 	struct ptp_clock *pclock;
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index dc398c6b7528..9c25d897be19 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -225,7 +225,7 @@ static ssize_t n_vclocks_store(struct device *dev,
 		for (i = 0; i < num - ptp->n_vclocks; i++) {
 			vclock = ptp_vclock_register(ptp);
 			if (!vclock)
-				goto out;
+				goto err_register;
 
 			*(ptp->vclock_index + ptp->n_vclocks + i) =
 				vclock->clock->index;
@@ -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);
+	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 84cb527f59cc..6378e9a8cd80 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;
+}
+
 static int ptp_vclock_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
 	s64 adj;
+	int err;
 
 	adj = (s64)scaled_ppm << PTP_VCLOCK_FADJ_SHIFT;
 	adj = div_s64(adj, PTP_VCLOCK_FADJ_DENOMINATOR);
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	timecounter_read(&vclock->tc);
-	vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj;
+	err = ptp_vclock_sample(vclock);
+	if (!err) {
+		timecounter_read(&vclock->tc);
+		vclock->cc.mult = PTP_VCLOCK_CC_MULT + adj;
+	}
 	mutex_unlock(&vclock->lock);
 
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_adjtime(struct ptp_clock_info *ptp, s64 delta)
@@ -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;
 }
 
 static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
@@ -115,13 +139,16 @@ static int ptp_vclock_settime(struct ptp_clock_info *ptp,
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
 	u64 ns = timespec64_to_ns(ts);
+	int err;
 
 	if (mutex_lock_interruptible(&vclock->lock))
 		return -EINTR;
-	timecounter_init(&vclock->tc, &vclock->cc, ns);
+	err = ptp_vclock_sample(vclock);
+	if (!err)
+		timecounter_init(&vclock->tc, &vclock->cc, ns);
 	mutex_unlock(&vclock->lock);
 
-	return 0;
+	return err;
 }
 
 static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
@@ -174,12 +201,8 @@ static const struct ptp_clock_info ptp_vclock_info = {
 static u64 ptp_vclock_read(struct cyclecounter *cc)
 {
 	struct ptp_vclock *vclock = cc_to_vclock(cc);
-	struct ptp_clock *ptp = vclock->pclock;
-	struct timespec64 ts = {};
-
-	ptp->info->getcycles64(ptp->info, &ts);
 
-	return timespec64_to_ns(&ts);
+	return vclock->cycles;
 }
 
 static const struct cyclecounter ptp_vclock_cc = {
@@ -214,6 +237,12 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	mutex_init(&vclock->lock);
 
+	if (ptp_vclock_sample(vclock)) {
+		kfree(vclock);
+		return NULL;
+	}
+	timecounter_init(&vclock->tc, &vclock->cc, 0);
+
 	vclock->clock = ptp_clock_register(&vclock->info, &pclock->dev);
 	if (IS_ERR_OR_NULL(vclock->clock)) {
 		kfree(vclock);
@@ -222,7 +251,6 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	ptp_vclock_set_subclass(vclock->clock);
 
-	timecounter_init(&vclock->tc, &vclock->cc, 0);
 	ptp_schedule_worker(vclock->clock, PTP_VCLOCK_REFRESH_INTERVAL);
 
 	ptp_vclock_hash_add(vclock);

---
base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7
change-id: 20260930-ptp-vclock-sampling-902f98408c93

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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