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, 05 Dec 2023 20:48:19 +0100
From: Johannes Berg <johannes@...solutions.net>
To: 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, Heiner Kallweit <hkallweit1@...il.com>, 
	Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Subject: Re: [RFC PATCH] net: ethtool: do runtime PM outside RTNL

On Tue, 2023-12-05 at 06:19 +0100, Przemek Kitszel wrote:
> 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)

I guess.

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

Well Jakub was arguing igc shouldn't be taking rtnl in suspend/resume,
maybe, but dunno.

> With your patch fixing the bug, there is
> no point with waiting IMO, so
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

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?

johannes