netdev - Re: [RFC 0/5] devlink: add an explicit locking API

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: <YYLgzVXO6IYoQkW9@nanopsycho>
Date:   Wed, 3 Nov 2021 20:19:41 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     leon@...nel.org, idosch@...sch.org, edwin.peer@...adcom.com,
        netdev@...r.kernel.org
Subject: Re: [RFC 0/5] devlink: add an explicit locking API

Wed, Nov 03, 2021 at 03:52:31PM CET, kuba@...nel.org wrote:
>On Wed, 3 Nov 2021 10:03:32 +0100 Jiri Pirko wrote:
>> Hi Jakub.
>> 
>> I took my time to read this thread and talked with Leon as well.
>> My original intention of locking in devlink was to maintain the locking
>> inside devlink.c to avoid the rtnl_lock-scenario.
>> 
>> However, I always feared that eventually we'll get to the point,
>> when it won't be possible to maintain any longer. I think may be it.
>
>Indeed, the two things I think we can avoid from rtnl_lock pitfalls 
>is that lock should be per-instance and that we should not wait for
>all refs to be gone at unregister time.
>
>Both are rather trivial to achieve with devlink.
>
>> In general, I like your approach. It is very clean and explicit. The
>> driver knows what to do, in which context it is and it can behave
>> accordingly. In theory or course, but the reality of drivers code tells
>> us often something different :)
>
>Right. I'll convert a few more drivers but the real test will be
>seeing if potential races are gone - hard to measure.
>
>> One small thing I don't fully undestand is the "opt-out" scenario which
>> makes things a bit tangled. But perhaps you can explain it a bit more.
>
>Do you mean the .lock_flags? That's a transitional thing so that people
>can convert drivers callback by callback to make prettier patch sets. 
>I may collapse all those flags into one, remains to be seen how useful
>it is when I create proper patches. This RFC is more of a code dump.
>
>The whole opt-out is to create the entire new API at once, and then
>convert drivers one-by-one (or allow the maintainers who care to do 
>it themselves). I find that easier and more friendly than slicing 
>the API and drivers multiple times.
>
>Long story short I expect the opt-out would be gone by the time 5.17
>merge window rolls around.

Okay, got it.


>
>> Leon claims that he thinks that he would be able to solve the locking
>> scheme leaving all locking internal to devlink.c. I suggest to give
>> him a week or 2 to present the solution. If he is not successful, lets
>> continue on your approach.
>> 
>> What do you think?
>
>I do worry a little bit that our goals differ. Seems like Leon wants to
>fix devlink for devlink and I want drivers to be able to lean on it.
>
>But I'm not attached to the exact approach or code, so as long as
>nobody is attached to theirs more RFCs can only help.
>
>Please be courteous and send as RFCs, tho.

Makes sense. Thanks!