netdev - Re: [PATCH net-next 09/11] devlink: convert occ

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:   Tue, 3 Apr 2018 18:47:19 -0600
From:   David Ahern <dsahern@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org,
        davem@...emloft.net, jiri@...lanox.com, petrm@...lanox.com,
        mlxsw@...lanox.com
Subject: Re: [PATCH net-next 09/11] devlink: convert occ_get op to separate
 registration

On 4/3/18 9:30 AM, Jiri Pirko wrote:
> Tue, Apr 03, 2018 at 04:33:11PM CEST, dsahern@...il.com wrote:
>> On 4/3/18 1:32 AM, Jiri Pirko wrote:
>>> Fri, Mar 30, 2018 at 04:45:50PM CEST, dsahern@...il.com wrote:
>>>> On 3/29/18 2:33 PM, Ido Schimmel wrote:
>>>>> From: Jiri Pirko <jiri@...lanox.com>
>>>>>
>>>>> This resolves race during initialization where the resources with
>>>>> ops are registered before driver and the structures used by occ_get
>>>>> op is initialized. So keep occ_get callbacks registered only when
>>>>> all structs are initialized.
>>>>
>>>> Why can't the occ_get handler look at some flag in an mlxsw struct to
>>>> know if the system has initialized?
>>>>
>>>> Separate registration here is awkward. You register a resource and then
>>>> register its op later.
>>>
>>> The separation is exactly why this patch is made. Note that devlink
>>> resouce is registered by core way before the initialization is done and
>>> the driver is actually able to perform the op. Also consider "reload"
>>
>> That's how you have chose to code it. I hit this problem adding devlink
>> to netdevsim; the solution was to fix the init order.
> 
> This is not about init order, at all. On reaload netdevs and internal
> driver structures disappear and appear again. And in between currently,
> the op is working with memory which is freed. That's the reason for this
> patch.
> 
> 
>>
>>> case, when the resource is still registered and the driver unloads and
>>> loads again. For that makes perfect sense to have that separated.
>>> Flag would just make things odd. Also, the priv could not be used in
>>> that case.
>>>
>>
>> I am not aware of any other API where you invoked the register function
>> at point A and then later add the operations at point B. In every API
>> that comes to mind the ops are part of the register.
> 
> I think that you just don't see any similar API.
> 
> 
>>
>> I am sure there are options for you to fix the init order of mlxsw
>> without making the devlink API awkward.
> 
> Again, not about init order, at all. I have no clue why you think so...
> 

Jiri, I am not aware of any other API where a driver registers with it
yet doesn't want the handler to be called so either waits to register
the handler later or unregisters the handler. That is an awkward design.
There are other options to have a sane API and handle the conditions you
need.