On 2026-07-27 13:15:14-07:00, Jakub Kicinski 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.
> ---
> ptp: Add ioctls for PHC timestamps with quality attributes
>
> This introduces PTP_SYS_OFFSET_EXTENDED_ATTRS and
> PTP_SYS_OFFSET_PRECISE_ATTRS, extending the existing extended/precise
> offset ioctls with clock quality attributes (error bound, timescale,
> sync status) and raw system counter fields. New driver callbacks
> gettimexattrs64 and getcrosststampattrs are added to ptp_clock_info.
>
> Link: https://lore.kernel.org/netdev/[email protected]/
> Link: https://lore.kernel.org/all/87se7ht25o.ffs@tglx/
>
> A few questions inline below.
>
> > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> > index dc23cd708cfe..d30ee4f51fbb 100644
> > --- a/drivers/ptp/ptp_chardev.c
> > +++ b/drivers/ptp/ptp_chardev.c
> > @@ -190,6 +190,8 @@ static long ptp_clock_getcaps(struct ptp_clock *ptp,
> > void __user *arg)
> > .cross_timestamping = ptp->info->getcrosststamp != NULL,
> > .adjust_phase = ptp->info->adjphase != NULL &&
> > ptp->info->getmaxphase != NULL,
> > + .extended_attrs = ptp->info->gettimexattrs64 != NULL,
> > + .precise_attrs = ptp->info->getcrosststampattrs !=
> > NULL,
> > };
>
> [Medium]
> Are the capability bits consistent with what the ioctls will actually
> accept?
>
> The caps are set based only on the presence of the new *_attrs callbacks,
> but the ioctl handlers fall back to gettimex64/getcrosststamp when the
> *_attrs callback is absent:
>
> if (ptp->info->gettimexattrs64)
> err = ptp->info->gettimexattrs64(ptp->info, &ts, &sts, &att);
> else if (ptp->info->gettimex64)
> err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> else
> return -EOPNOTSUPP;
>
> A userspace that respects the capability bits would refuse to call the
> new ioctl on drivers that would in fact serve it via the fallback. Is
> the fallback intended, and if so, should the caps advertise it? The
> commit message does not describe this behavior.
>
The fallback is intentional. I'll widen the caps in v6 so they advertise it
(gettimexattrs64 || gettimex64, getcrosststampattrs || getcrosststamp) and
document the behavior in the commit message. Thanks.
> [ ... ]
>
> > @@ -404,6 +413,130 @@ static long ptp_sys_offset_extended(struct ptp_clock
> > *ptp, void __user *arg,
> > return copy_to_user(arg, extoff, sizeof(*extoff)) ? -EFAULT : 0;
> > }
> >
> > +static long ptp_sys_offset_extended_attrs(struct ptp_clock *ptp, void
> > __user *arg)
> > +{
> > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
> > + struct ptp_attrs_request request;
> > + struct ptp_system_timestamp sts;
>
> [Medium]
> Can this leak uninitialized stack contents to userspace?
>
> Only sts.clockid is initialized below, and sts is reused across loop
> iterations. If any driver's gettimex64/gettimexattrs64 returns success
> without touching pre_sts/post_sts on the sts pointer, sts.pre_sts.valid
> is either uninitialized (first iteration) or stale (later iterations),
> and the "if (!sts.pre_sts.valid || !sts.post_sts.valid)" gate can admit
> a partially-populated snapshot.
>
> On the success path the code then copies pre_sts.cycles, pre_sts.cs_id,
> pre_sts.monoraw, pre_sts.systime and the post_sts equivalents to
> userspace. The pre-existing ptp_sys_offset_extended() has the same
> shape but only copied systime, so the exposed surface is now wider.
> Would something like:
>
> struct ptp_system_timestamp sts = {};
>
> inside the loop (or at declaration) be safer?
>
Yes — I'll zero-initialize sts per loop iteration in v6, so nothing
uninitialized or stale can reach userspace. Thanks.
> > + unsigned int n_samples;
> > + int err;
> > +
> > + if (copy_from_user(&request, arg, sizeof(request)))
> > + return -EFAULT;
> > +
> > + if (request.valid ||
> > + request.num_samples > PTP_MAX_SAMPLES ||
> > + request.num_samples == 0)
> > + return -EINVAL;
>
> [Medium]
> Should this also reject non-zero request.rsv[]?
>
> The kernel-doc for struct ptp_attrs_request says "Reserved for future
> use, must be zero", but rsv[3] is never validated. The neighboring
> ptp_sys_offset_extended() enforces the same contract on its own
> reserved fields:
>
> if (extoff->n_samples > PTP_MAX_SAMPLES || extoff->rsv[0] ||
> extoff->rsv[1])
> return -EINVAL;
>
> Without a similar check, existing userspace binaries can start sending
> garbage in rsv[], which then blocks any future repurposing of those
> bytes.
>
Agreed — v6 will reject a non-zero rsv[], matching
ptp_sys_offset_extended(). Thanks.
> > +
> > + err = ptp_validate_sys_offset_clockid(request.clock_id);
> > + if (err)
> > + return err;
> > +
> > + n_samples = request.num_samples;
> > + sts.clockid = request.clock_id;
> > +
> > + data = kzalloc(struct_size(data, timestamps, n_samples), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->request.num_samples = n_samples;
>
> [Low]
> This is a _IOWR ioctl and the whole struct (including data->request) is
> copied back to userspace at the end, but only data->request.num_samples
> is populated. data->request.valid, data->request.clock_id and
> data->request.rsv[] therefore always come back as zero, silently
> overwriting the values userspace put in the request buffer.
>
> Is that intentional? It is not mentioned in the commit message, and it
> locks in a slightly surprising contract for future extensions.
>
Not intentional — v6 will echo the full request back so
valid/clock_id/num_samples/rsv round-trip as submitted. Thanks.
> > +
> > + for (unsigned int i = 0; i < n_samples; i++) {
> > + struct ptp_clock_attrs att = {};
> > + struct timespec64 ts;
> > +
> > + if (ptp->info->gettimexattrs64)
> > + err = ptp->info->gettimexattrs64(ptp->info, &ts,
> > + &sts, &att);
> > + else if (ptp->info->gettimex64)
> > + err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> > + else
> > + return -EOPNOTSUPP;
> > +
> > + if (err)
> > + return err;
> > +
> > + /* Filter out disabled or unavailable clocks */
> > + if (!sts.pre_sts.valid || !sts.post_sts.valid)
> > + return -EINVAL;
> > +
> > + data->timestamps[i].pre_systime.sys_time =
> > + ktime_to_ns(sts.pre_sts.systime);
> > + data->timestamps[i].pre_systime.sys_rawtime =
> > + ktime_to_ns(sts.pre_sts.monoraw);
> > + data->timestamps[i].pre_systime.sys_counter =
> > + sts.pre_sts.cycles;
> > + data->timestamps[i].pre_systime.sys_counter_id =
> > + sts.pre_sts.cs_id;
>
> [Critical]
> Does the sys_counter_id assignment produce the correct UAPI value?
>
> sts.pre_sts.cs_id is enum clocksource_ids from
> include/linux/clocksource_ids.h:
>
> CSID_GENERIC = 0,
> CSID_ARM_ARCH_COUNTER = 1,
> CSID_S390_TOD = 2,
> CSID_X86_TSC_EARLY = 3,
> CSID_X86_TSC = 4,
> CSID_X86_KVM_CLK = 5,
> CSID_X86_ART = 6,
>
> But sys_counter_id is documented as enum ptp_counter_id from the UAPI:
>
> PTP_COUNTER_UNKNOWN = 0,
> PTP_COUNTER_X86_TSC = 1,
> PTP_COUNTER_ARM_ARCH = 2,
>
> With this direct assignment, on ARM sts.pre_sts.cs_id == 1
> (CSID_ARM_ARCH_COUNTER) is reported to userspace as PTP_COUNTER_X86_TSC.
> On x86, real CSID_X86_TSC (4) has no defined UAPI meaning. On s390,
> CSID_S390_TOD (2) is reported as PTP_COUNTER_ARM_ARCH.
>
> Should the ioctl translate cs_id into the UAPI enum before copying it
> out? Once released, later adding a translation layer would break any
> userspace that started decoding the raw kernel values.
>
Good catch — that's a bug. v6 will translate enum clocksource_ids to the
UAPI enum ptp_counter_id before copying out. Thanks.
> [High]
> On clocksources that are derived/virtualised (kvm-clock, scaled arch
> counter, ART, ...), the raw underlying counter identity is exposed by
> system_time_snapshot as hw_cycles/hw_csid rather than cycles/cs_id.
> The new UAPI only propagates cycles/cs_id.
>
> The commit message says one motivation is:
>
> 3. VMMs that need raw system counter values paired with PTP
> timestamps for feed-forward clock calibration ...
>
> but a guest on a derived clocksource will not see the underlying
> hardware counter identity or value here. Should the UAPI also expose
> hw_cycles/hw_csid, given that the struct is frozen once merged?
>
This is being handled as a follow-up (David Woodhouse offered on-list to
take the hw_csid side). This series exposes the base cs_id/cycles, and the
UAPI leaves room to add the hw_* values without an ABI change.
> > + data->timestamps[i].devtime.device_time.sec = ts.tv_sec;
> > + data->timestamps[i].devtime.device_time.nsec = ts.tv_nsec;
> > + data->timestamps[i].devtime.attrs = att;
>
> [ ... ]
>
> > +}
> > +
> > +static long ptp_sys_offset_precise_attrs(struct ptp_clock *ptp, void
> > __user *arg)
> > +{
> > + struct ptp_sys_offset_attrs *data __free(kfree) = NULL;
> > + struct system_device_crosststamp xtstamp;
> > + struct ptp_attrs_request request;
> > + struct ptp_clock_attrs att = {};
> > + struct timespec64 ts;
> > + int err;
> > +
> > + if (copy_from_user(&request, arg, sizeof(request)))
> > + return -EFAULT;
> > +
> > + if (request.valid ||
> > + request.num_samples != 1)
> > + return -EINVAL;
> > +
> > + err = ptp_validate_sys_offset_clockid(request.clock_id);
> > + if (err)
> > + return err;
> > +
> > + data = kzalloc(struct_size(data, timestamps, 1), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + if (ptp->info->getcrosststampattrs)
> > + err = ptp->info->getcrosststampattrs(ptp->info, &xtstamp, &att);
> > + else if (ptp->info->getcrosststamp)
> > + err = ptp->info->getcrosststamp(ptp->info, &xtstamp);
> > + else
> > + return -EOPNOTSUPP;
>
> [High]
> Can any driver using get_device_system_crosststamp() actually serve
> this ioctl?
>
> xtstamp is declared without an initializer, so xtstamp.clock_id holds
> whatever was on the stack. The pre-existing ptp_sys_offset_precise()
> explicitly sets it:
>
> struct system_device_crosststamp xtstamp = {
> .clock_id = CLOCK_REALTIME,
> };
>
> Drivers commonly forward xtstamp to get_device_system_crosststamp() in
> kernel/time/timekeeping.c, which switches on xtstamp->clock_id and
> falls through to:
>
> default:
> WARN_ON_ONCE(1);
> return -ENODEV;
>
> So on most drivers implementing getcrosststamp (kvm, mlx5, ice, igc,
> bnxt, s390, ...), an unprivileged caller of PTP_SYS_OFFSET_PRECISE_ATTRS
> would trigger a first-hit WARN and get -ENODEV.
>
> In addition, request.clock_id is validated by
> ptp_validate_sys_offset_clockid() but never assigned to
> xtstamp.clock_id, so the userspace-selected clock is silently dropped
> even for the paths documented in the UAPI header (CLOCK_MONOTONIC,
> CLOCK_MONOTONIC_RAW, CLOCK_AUX...).
>
> Should xtstamp.clock_id be set from request.clock_id (and xtstamp
> zeroed before use)?
>
Real bug, thanks. v6 zero-initializes xtstamp and sets clock_id from the
validated request. To avoid the get_device_system_crosststamp() default
WARN, precise will validate clock_id against a dedicated allowlist (REALTIME
+ AUX) and return -EINVAL for anything else.
> [ ... ]
>
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index d6f54ccaf93b..849aef8191c5 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -112,7 +112,9 @@ static int ptp_clock_gettime(struct posix_clock *pc,
> > struct timespec64 *tp)
> > struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
> > int err;
> >
> > - if (ptp->info->gettimex64)
> > + if (ptp->info->gettimexattrs64)
> > + err = ptp->info->gettimexattrs64(ptp->info, tp, NULL, NULL);
> > + else if (ptp->info->gettimex64)
> > err = ptp->info->gettimex64(ptp->info, tp, NULL);
> > else
> > err = ptp->info->gettime64(ptp->info, tp);
>
> [Low]
> This call site passes both sts and att as NULL, but the kernel-doc for
> gettimexattrs64 only documents that att may be NULL ("fills @att (if
> not NULL)"). The dual-NULL contract on sts is implicitly inherited
> from gettimex64 but is not stated.
>
> Would it be worth spelling out in the kernel-doc that both sts and att
> may be NULL, so a new driver author does not skip the NULL check on
> sts?
Yes — v6 will state that both sts and att may be NULL. Thanks.
>
> [ ... ]
