netdev - RE: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for E810 devices

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]

Message-ID: <427ddb2579f14d77b537aae9c2fa9759@intel.com>
Date:   Mon, 14 Jun 2021 19:50:23 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        Richard Cochran <richardcochran@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Brelinski, TonyX" <tonyx.brelinski@...el.com>
Subject: RE: [PATCH net-next 5/8] ice: register 1588 PTP clock device object
 for E810 devices



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, June 14, 2021 11:09 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Richard Cochran
> <richardcochran@...il.com>; davem@...emloft.net; netdev@...r.kernel.org;
> sassmann@...hat.com; Brelinski, TonyX <tonyx.brelinski@...el.com>
> Subject: Re: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for
> E810 devices
> 
> On Mon, 14 Jun 2021 09:43:17 -0700 Jacob Keller wrote:
> > >> +static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
> > >> +{
> > >> +	struct ice_pf *pf = ptp_info_to_pf(info);
> > >> +	u64 freq, divisor = 1000000ULL;
> > >> +	struct ice_hw *hw = &pf->hw;
> > >> +	s64 incval, diff;
> > >> +	int neg_adj = 0;
> > >> +	int err;
> > >> +
> > >> +	incval = ICE_PTP_NOMINAL_INCVAL_E810;
> > >> +
> > >> +	if (scaled_ppm < 0) {
> > >> +		neg_adj = 1;
> > >> +		scaled_ppm = -scaled_ppm;
> > >> +	}
> > >> +
> > >> +	while ((u64)scaled_ppm > div_u64(U64_MAX, incval)) {
> > >> +		/* handle overflow by scaling down the scaled_ppm and
> > >> +		 * the divisor, losing some precision
> > >> +		 */
> > >> +		scaled_ppm >>= 2;
> > >> +		divisor >>= 2;
> > >> +	}
> > >
> > > I have a question regarding ppm overflows.
> > >
> > > We have the max_adj field in struct ptp_clock_info which is checked
> > > against ppb, but ppb is a signed 32 bit and scaled_ppm is a long,
> > > meaning values larger than S32_MAX << 16 / 1000 will overflow
> > > the ppb calculation, and therefore the check.
> >
> > Hmmm.. I thought ppb was a s64, not an s32.
> >
> > In general, I believe max_adj is usually capped at 1 billion anyways,
> > since it doesn't make sense to slow a clock by more than 1billioln ppb,
> > and increasing it more than that isn't really useful either.
> 
> Do you mean it's capped somewhere in the code to 1B?
> 
> I'm no time expert but this is not probability where 1 is a magic
> value, adjusting clock by 1 - 1ppb vs 1 + 1ppb makes little difference,
> no? Both mean something is super fishy with the nominal or expected
> frequency, but the hardware can do that and more.
> 
> Flipping the question, if adjusting by large ppb values is not correct,
> why not cap the adjustment at the value which would prevent the u64
> overflow?

Large ppb values are sometimes used when you want to slew a clock to bring it in sync when its a few milliseconds to seconds off, without performing a time jump (so that you maintain monotonic increasing time).

That being said, we are supposed to be checking max_adj, except that you're right the conversion to ppb could overflow, and there's no check prior to the conversion from scaled_ppm to ppb.

> 
> I don't really have a preferences here, I'm mostly disturbed by
> the overflow in the ppb vs max_adj check.
> 
> > > Are we okay with that? Is my math off? Did I miss some part
> > > of the kernel which filters crazy high scaled_ppm/freq?
> > >
> > > Since dialed_freq is updated regardless of return value of .adjfine
> > > the driver has no clear way to reject bad scaled_ppm>
> >
> > I'm not sure. +Richard?