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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 14 Jun 2021 13:51:57 -0700
From:   Jacob Keller <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



On 6/14/2021 1:48 PM, Jakub Kicinski wrote:
> On Mon, 14 Jun 2021 19:50:23 +0000 Keller, Jacob E wrote:
>>>> 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).
> 
> Ah, you're right, ptp4l will explicitly cap the freq adjustments
> based on max_adj from sysfs, so setting max_adj too low could impact
> the convergence time in strange scenarios.
> 

Your patch to fix it so that the conversion from scaled_ppm to ppb can't
overflow is the correct approach, here. The scaled_ppm function didn't
account for the fact that the provided adjustment could overflow the s32.

Increasing that to s64 ensures it won't overflow and prevents invalid
bogus frequencies from passing that check.