netdev - Re: [RFC PATCH net-next 1/4] net-sysfs: remove rtnl

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

Message-ID: <173703457791.6390.1011724914365700977@kwain>
Date: Thu, 16 Jan 2025 14:36:17 +0100
From: Antoine Tenart <atenart@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com, netdev@...r.kernel.org, gregkh@...uxfoundation.org, mhocko@...e.com, stephen@...workplumber.org
Subject: Re: [RFC PATCH net-next 1/4] net-sysfs: remove rtnl_trylock from device attributes

Hi Jakub,

Quoting Jakub Kicinski (2025-01-07 18:06:41)
> On Tue, 07 Jan 2025 17:30:03 +0100 Antoine Tenart wrote:
> > Quoting Jakub Kicinski (2025-01-02 23:36:47)
> > > My version, FWIW:
> > > https://github.com/kuba-moo/linux/commit/2724bb7275496a254b001fe06fe20ccc5addc9d2  
> > 
> > I might take a few of your changes in there, eg. I see you used an
> > interruptible lock. With this and the few minors comments this RFC got I
> > can prepare a new series.
> 
> Perfect.

While refreshing the series, especially after adding the dev_isalive()
check, I found out we actually do not need to drop the sysfs protection
and hold a reference to the net device during the whole rtnl locking
section. This is because after getting the rtnl lock and once we know
the net device dismantle hasn't started yet, we're sure dismantle won't
start (and the device won't be freed) until we give back the rtnl lock.

This makes the new helpers easier to use, does not require to expose
the kernfs node to users, making the code more contained; but the
locking order is not as perfect.

We would go from (version 1),

1. unlocking sysfs
2. locking rtnl
3. unlocking rtnl
4. locking sysfs

to (version 2),

1. unlocking sysfs
2. locking rtnl
3. locking sysfs
4. unlocking rtnl

This is actually fine because the "sysfs lock" isn't a lock but a
refcnt, with the only deadlock situation being when draining it.

Version 1: https://github.com/atenart/linux/commit/596c5d9895ccdb75057978abd6be1a42ee4b448e
Version 2: https://github.com/atenart/linux/commit/c6659bb26f564f1fd63d1c279616f57141e9f2bf

Thoughts? Apart from that question, either series is ready for
submission.

Thanks,
Antoine