netdev - Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

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

Date: Fri, 21 Jun 2024 06:16:21 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, 
	Ziwei Xiao <ziweixiao@...gle.com>, Praveen Kaligineedi <pkaligineedi@...gle.com>, 
	Harshitha Ramamurthy <hramamurthy@...gle.com>, Willem de Bruijn <willemb@...gle.com>, 
	Jeroen de Borst <jeroendb@...gle.com>, Shailend Chand <shailend@...gle.com>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com
Subject: Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of
 rtnl lock

On Fri, Jun 21, 2024 at 2:22 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 20 Jun 2024 11:47:08 +0000 Eric Dumazet wrote:
> > Move pm_runtime_get_sync() and pm_runtime_put() out of __dev_ethtool
> > to dev_ethtool() while RTNL is not yet held.
> >
> > These helpers do not depend on RTNL.
>
> The helpers themselves don't, but can we assume no drivers have
> implicit dependencies on calling netif_device_detach() under rtnl_lock,
> and since the presence checks are under rtnl_lock they are currently
> guaranteed not to get any callbacks past detach() + rtnl_unlock()?
>
> I think its better to completely skip PM + presence + ->begin if driver
> wants the op to be unlocked, but otherwise keep the locking as is

This PM stuff came 3 years ago, for apparently lack of user space awareness.

commit f32a213765739f2a1db319346799f130a3d08820
Author: Heiner Kallweit <hkallweit1@...il.com>
Date:   Sun Aug 1 12:36:48 2021 +0200

    ethtool: runtime-resume netdev parent before ethtool ioctl ops

I have not looked closely at the ->begin() and ->close() stuff, I will
do this next week (I am OOO this Friday)

>
> I also keep wondering whether we shouldn't use this as an opportunity
> to introduce a "netdev instance lock". I think you mentioned we should
> move away from rtnl for locking ethtool and ndos since most drivers
> don't care at all about global state. Doing that is a huge project,
> but maybe this is where we start?

Yes, a per device mutex would probably be needed in the long term.