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

Message-ID: <20211103075231.0a53330c@kicinski-fedora-PC1C0HJN>
Date:   Wed, 3 Nov 2021 07:52:31 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
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

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.

> 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.