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. [ ... ] > @@ -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? > + 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. > + > + 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. > + > + 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. [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? > + 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)? [ ... ] > 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? [ ... ] -- pw-bot: cr
