netdev - Re: [RFC PATCH] net: ethtool: do runtime PM outside 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <64684afc-3dbb-453e-9c90-bf2a86e70c50@gmail.com>
Date: Wed, 6 Dec 2023 12:59:09 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Johannes Berg <johannes@...solutions.net>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>, netdev@...r.kernel.org
Cc: Marc MERLIN <marc@...lins.org>,
 Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>, intel-wired-lan@...ts.osuosl.org,
 Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Subject: Re: [RFC PATCH] net: ethtool: do runtime PM outside RTNL

On 06.12.2023 10:37, Johannes Berg wrote:
> On Wed, 2023-12-06 at 09:46 +0100, Przemek Kitszel wrote:
>>
>> That sounds right too; one could argue if your fix is orthogonal to that
>> or not. I would say that your fix makes core net code more robust
>> against drivers from past millennia. :)
>> igc folks are notified, no idea how much time it would take to propose
>> a fix.
> 
> Maybe it should be on whoever added runtime pm to ethtool ;-)
> 
> Heiner, the igc driver was already doing this when you added
> pm_runtime_get_sync() ops, was there a discussion at the time, or just
> missed?
> 
I think it went unnoticed at that time that igc is acquiring RTNL
in runtime-resume. I'm just astonished that this pops up only now,
considering that the change was done more than 2 yrs ago.

Note: In __dev_open() there's a similar scenario where the
runtime-resume callback may be executed under RTNL.

> I really don't know any of this ...
> 
>>> Well, according to the checks, the patch really should use
>>> netdev_get_by_name() and netdev_put()? But I don't know how to do that
>>> on short-term stack thing ... maybe it doesn't have to?
>>
>> Nice to have such checks :)
>> You need some &netdevice_tracker, perhaps one added into struct net
>> or other place that would allow to track it at ethtool level.
> 
> Yeah but that's dynamic? Seems weird to add something to allocations for
> something released in the same function ...
> 
>> "short term stack thing" does not relieve us from good coding practices,
>> but perhaps "you just replaced __dev_get_by_name() call by
>> dev_get_by_name()" to fix a bug would ;) - with transition to tracked
>> alloc as a next series to be promised :)
> 
> All I want is to know how ;)
> but I guess I can try to find examples.
> 
>> anyway, I'm fresh here, and would love to know what others think about
> 
> Not me, but me too ;-)
> 
> johannes