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]

Date: Tue, 5 Dec 2023 06:19:15 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Johannes Berg <johannes@...solutions.net>, <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>, Heiner Kallweit <hkallweit1@...il.com>,
	Johannes Berg <johannes.berg@...el.com>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>
Subject: Re: [RFC PATCH] net: ethtool: do runtime PM outside RTNL

On 12/4/23 20:07, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.com>
> 
> As reported by Marc MERLIN in [1], at least one driver (igc)

perhaps Reported-by tag? (I know this is RFC as of now)

> wants/needs to acquire the RTNL inside suspend/resume ops,
> which can be called from here in ethtool if runtime PM is
> enabled.
> 
> [1] https://lore.kernel.org/r/20231202221402.GA11155@merlins.org
> 
> Allow this by doing runtime PM transitions without the RTNL
> held. For the ioctl to have the same operations order, this
> required reworking the code to separately check validity and
> do the operation. For the netlink code, this now has to do
> the runtime_pm_put a bit later.
> 
> Signed-off-by: Johannes Berg <johannes.berg@...el.com>
> ---
>   net/ethtool/ioctl.c   | 71 ++++++++++++++++++++++++++-----------------
>   net/ethtool/netlink.c | 32 ++++++++-----------
>   2 files changed, 56 insertions(+), 47 deletions(-)
> 
Thank you for the patch,

I like the idea of split into validate + do for dev_ethtool(),
what minimizes unneeded PM touching. Moving pm_runtime_get_sync() out of 
RTNL is also a great improvement per se. Also from the pure coding 
perspective I see no obvious flaws in the patch. I think that igc code
was just accidental to the issue, in a way that it was not deliberate to
hold RTNL for extended periods. With your patch fixing the bug, there is
no point with waiting IMO, so

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>