netdev - Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 09 Jun 2023 08:38:11 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Jacob
 Keller <jacob.e.keller@...el.com>, Gal Pressman <gal@...dia.com>, Tariq
 Toukan <tariqt@...dia.com>,  Saeed Mahameed <saeed@...nel.org>, Richard
 Cochran <richardcochran@...il.com>, Vincent Cheng
 <vincent.cheng.xh@...esas.com>
Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase
 ptp_clock_info callback

Hi,

I'm sorry for the late reply. This fell under my radar.

On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote:
> On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@...hat.com> wrote:
> > On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote:
> > > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> > > > index c9d451bf89e2..f6f9d4adce04 100644
> > > > --- a/drivers/ptp/ptp_clockmatrix.c
> > > > +++ b/drivers/ptp/ptp_clockmatrix.c
> > > > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel)
> > > >  /* PTP Hardware Clock interface */
> > > >  
> > > >  /*
> > > > - * Maximum absolute value for write phase offset in picoseconds
> > > > - *
> > > > - * @channel:  channel
> > > > - * @delta_ns: delta in nanoseconds
> > > > + * Maximum absolute value for write phase offset in nanoseconds
> > > >   *
> > > >   * Destination signed register is 32-bit register in resolution of 50ps
> > > >   *
> > > > - * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350
> > > > + * 0x7fffffff * 50 =  2147483647 * 50 = 107374182350 ps
> > > > + * Represent 107374182350 ps as 107374182 ns
> > > > + */
> > > > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused)
> > > > +{
> > > > +	return MAX_ABS_WRITE_PHASE_NANOSECONDS;
> > > > +}
> > > 
> > > This introduces a functional change WRT the current code. Prior to this
> > > patch ClockMatrix tries to adjust phase delta even above
> > > MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value.
> > > After this patch it will error out.
> 
> My understanding is the syscall for adjphase, clock_adjtime, cannot
> represent an offset granularity smaller than nanoseconds using the
> struct timex offset member. 

Ok.

> To me, it seems that adjusting a delta above
> MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision
> units by the device), while supported by the device driver, would not be
> a capability utilized by any interface that would invoke the .adjphase
> callback implemented by ClockMatrix.

Here I don't follow. I must admit I know the ptp subsystem very little,
but AFAICS, we could have e.g.

clock_adjtime() // offset > 200 secs (200000000 usec)
 -> do_clock_adjtime
    -> kc->clock_adj
       -> clock_posix_dynamic
          -> pc_clock_adjtime
             -> ptp_clock_adjtime
                -> _idtcm_adjphase // delta land unmodified up here

I guess the user-space could pass such large delta (e.g. at boot
time?!?). If so, with this patch we change an user-space observable
behavior, and I think we should avoid that.

Thanks

Paolo