netdev - Re: [PATCH 0/3] using guard/_

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: <b8874290-905c-45ef-92c5-897b66c1f8d0@intel.com>
Date: Wed, 27 Mar 2024 12:15:28 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Johannes Berg
	<johannes@...solutions.net>
CC: <netdev@...r.kernel.org>
Subject: Re: [PATCH 0/3] using guard/__free in networking

On 3/26/24 15:37, Jakub Kicinski wrote:
> On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
>> On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
>>> On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
>>>> Hi,
>>>>
>>>> So I started playing with this for wifi, and overall that
>>>> does look pretty nice, but it's a bit weird if we can do
>>>>
>>>>    guard(wiphy)(&rdev->wiphy);
>>>>
>>>> or so, but still have to manually handle the RTNL in the
>>>> same code.
>>>
>>> Dunno, it locks code instead of data accesses.
>>
>> Well, I'm not sure that's a fair complaint. After all, without any more
>> compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
>> Clearly
>>
>> 	rtnl_lock();
>> 	// something
>> 	rtnl_unlock();
>>
>> also locks the "// something" code, after all., and yeah that might be
>> doing data accesses, but it might also be a function call or a whole
>> bunch of other things?
>>
>> Or if you look at something like bpf_xdp_link_attach(), I don't think
>> you can really say that it locks only data. That doesn't even do the
>> allocation outside the lock (though I did convert that one to
>> scoped_guard because of that.)
>>
>> Or even something simple like unregister_netdev(), it just requires the
>> RTNL for some data accesses and consistency deep inside
>> unregister_netdevice(), not for any specific data accessed there.
>>
>> So yeah, this is always going to be a trade-off, but all the locking is.
>> We even make similar trade-offs manually, e.g. look at
>> bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
>> still, for no good reason other than simplifying the cleanup path there.
> 
> At least to me the mental model is different. 99% of the time the guard
> is covering the entire body. So now we're moving from "I'm touching X
> so I need to lock" to "This _function_ is safe to touch X".
> 
>> Anyway, I can live with it either way (unless you tell me you won't pull
>> wireless code using guard), just thought doing the wireless locking with
>> guard and the RTNL around it without it (only in a few places do we
>> still use RTNL though) looked odd.
>>
>>
>>> Forgive the comparison but it feels too much like Java to me :)
>>
>> Heh. Haven't used Java in 20 years or so...
> 
> I only did at uni, but I think they had a decorator for a method, where
> you can basically say "this method should be under lock X" and runtime
> will take that lock before entering and drop it after exit,
> appropriately. I wonder why the sudden love for this concept :S
> Is it also present in Rust or some such?

:D
There is indeed a lot of proposals around the topic lately :)

I believe that "The first half of the 6.8 merge window" [lwn] article
has brought attention to the in-kernel "scope-based resource management"
availability.

[lwn] https://lwn.net/Articles/957188/

More abstraction/sugar over __free() is cleanly needed to have code both
easier to follow and less buggy.

You made a good point to encourage scoping the locks to small blocks
instead of whole functions. And "less typing" to instantiate the lock
guard variable is one way to do that.

> 
>>> scoped_guard is fine, the guard() not so much.
>>
>> I think you can't get scoped_guard() without guard(), so does that mean
>> you'd accept the first patch in the series?
> 
> How can we get one without the other.. do you reckon Joe P would let us
> add a checkpatch check to warn people against pure guard() under net/ ?
> 
>>> Do you have a piece of code in wireless where the conversion
>>> made you go "wow, this is so much cleaner"?
>>
>> Mostly long and complex error paths. Found a double-unlock bug (in
>> iwlwifi) too, when converting some locking there.
>>
>> Doing a more broader conversion on cfg80211/mac80211 removes around 200
>> lines of unlocking, mostly error handling, code.
>>
>> Doing __free() too will probably clean up even more.
> 
> Not super convinced by that one either:
> https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> maybe I'm too conservative..
>