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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8eeae19a0535bfe72f87ee8c74a15dd2e753c765.camel@sipsolutions.net>
Date: Tue, 26 Mar 2024 09:42:43 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH 0/3] using guard/__free in networking

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.

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

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

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

johannes