Re: [PATCH v5 net-next 1/7] ptp: Add ioctls for PHC timestamps with quality attributes

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.

> 
> [ ... ]