netdev - Re: [PATCH net-next 05/12] ionic: add hw timestamp support files

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] [day] [month] [year] [list]

Message-ID: <fc385b43-06e4-d235-210a-337a846868bb@pensando.io>
Date:   Tue, 6 Apr 2021 21:55:40 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        drivers@...sando.io, Allen Hubbe <allenbh@...sando.io>
Subject: Re: [PATCH net-next 05/12] ionic: add hw timestamp support files

On 4/6/21 5:27 PM, Richard Cochran wrote:
> On Tue, Apr 06, 2021 at 04:18:00PM -0700, Shannon Nelson wrote:
>> On 4/5/21 11:17 AM, Richard Cochran wrote:
>>> On Mon, Apr 05, 2021 at 09:16:39AM -0700, Shannon Nelson wrote:
>>>> On 4/4/21 4:05 PM, Richard Cochran wrote:
>>>>> This check is unneeded, because the ioctl layer never passes NULL here.
>>>> Yes, the ioctl layer never calls this with NULL, but we call it from within
>>>> the driver when we spin operations back up after a FW reset.
>>> So why not avoid the special case and pass a proper request?
>> We do this because our firmware reset path is a special case that we have to
>> handle, and we do so by replaying the previous configuration request.
>> Passing the NULL request gives the code the ability to watch for this case
>> while keeping the special case handling simple: the code that drives the
>> replay logic doesn't need to know the hwstamp details, it just needs to
>> signal the replay and let the hwstamp code keep track of its own data and
>> request history.
>>
>> I can update the comment to make that replay case more obvious.
> No, please, I am asking you to provide a hwtstamp_config from your
> driver.  What is so hard about that?
>

What I think you are asking is that we not extend the current assumption 
that *ifr will always be a useful pointer, a perfectly reasonable 
request.  Our ioctl() handler follows this as expected. It is our 
internal usage that might look like an extension.

We'd like to keep the replay related code and data to a minimum, and 
this current implementation is a simple way to do so, keeping the state 
and config info within the ptp side.

I suppose one alternative is that we pull the copy_from_user() bits into 
ionic_ioctl() and hand a hwtstamp_config struct pointer to 
ionic_lif_hwstamp_set().  In our reset case we can either hand a NULL 
pointer to ionic_lif_hwstamp_set() or add a reset parameter to the call 
where ionic_lif_hwstamp_set() simply won't inspect the hwtstamp_config 
pointer.

Another alternative would be to split the top layer of 
ionic_lif_hwstamp_set() into two different functions, one called by the 
ioctl handler with an *ifr, the other called by the fw reset logic 
without an *ifr, that both call into the remaining with a 
hwtstamp_config pointer.  This seems somewhat similar to the ixgbe approach.

Either way, in the end it seems like extra code to get to the same result.

sln