netdev - Re: [PATCH net] 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] [day] [month] [year] [list]

Message-ID: <14be0220-65d6-d0cd-e0da-26bcab4d3d1c@intel.com>
Date: Wed, 6 Dec 2023 11:36:59 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Johannes Berg <johannes@...solutions.net>, <netdev@...r.kernel.org>
CC: Heiner Kallweit <hkallweit1@...il.com>, Johannes Berg
	<johannes.berg@...el.com>, Marc MERLIN <marc@...lins.org>
Subject: Re: [PATCH net] net: ethtool: do runtime PM outside RTNL

On 12/6/23 11:03, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.com>
> 
> As reported by Marc MERLIN, at least one driver (igc) wants or
> needs to acquire the RTNL inside suspend/resume ops, which can
> be called from here in ethtool if runtime PM is enabled.
> 
> 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.
> 
> Reported-by: Marc MERLIN <marc@...lins.org>
> Fixes: f32a21376573 ("ethtool: runtime-resume netdev parent before ethtool ioctl ops")
> Fixes: d43c65b05b84 ("ethtool: runtime-resume netdev parent in ethnl_ops_begin")
> Closes: https://lore.kernel.org/r/20231202221402.GA11155@merlins.org
> Signed-off-by: Johannes Berg <johannes.berg@...el.com>
> ---
> v2:
>   - add tags
>   - use netdev_get_by_name()/netdev_put() in ioctl path
> ---
>   net/ethtool/ioctl.c   | 72 ++++++++++++++++++++++++++-----------------
>   net/ethtool/netlink.c | 32 ++++++++-----------
>   2 files changed, 57 insertions(+), 47 deletions(-)
> 

[snip]

> @@ -3070,7 +3065,9 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
>   int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
>   {
>   	struct ethtool_devlink_compat *state;
> -	u32 ethcmd;
> +	struct net_device *dev = NULL;
> +	netdevice_tracker dev_tracker;

nice :)

> +	u32 ethcmd, subcmd;
>   	int rc;
>   
>   	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
> @@ -3090,9 +3087,26 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
>   		break;
>   	}
>   
> +	dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL);
> +	if (!dev) {
> +		rc = -ENODEV;
> +		goto exit_free;
> +	}
> +
> +	rc = __dev_ethtool_check(dev, useraddr, ethcmd, &subcmd);
> +	if (rc)
> +		goto exit_free;
> +
> +	if (dev->dev.parent)
> +		pm_runtime_get_sync(dev->dev.parent);
> +
>   	rtnl_lock();
> -	rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
> +	rc = __dev_ethtool_do(dev, ifr, useraddr, ethcmd, subcmd, state);
>   	rtnl_unlock();
> +
> +	if (dev->dev.parent)
> +		pm_runtime_put(dev->dev.parent);
> +
>   	if (rc)
>   		goto exit_free;
>   
> @@ -3115,6 +3129,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
>   	}
>   
>   exit_free:
> +	if (dev)
> +		netdev_put(dev, &dev_tracker);

this `if (dev)` check is the first line of netdev_put(), not needed here

>   	if (state->devlink)
>   		devlink_put(state->devlink);
>   	kfree(state);

[snip]

just a nitpick so,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>