netdev - Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent before ethtool ioctl ops

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 3 Aug 2021 23:41:23 +0300
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Linux PM list <linux-pm@...r.kernel.org>
Subject: Re: [PATCH net-next 1/4] ethtool: runtime-resume netdev parent before
 ethtool ioctl ops



On 01/08/2021 13:36, Heiner Kallweit wrote:
> If a network device is runtime-suspended then:
> - network device may be flagged as detached and all ethtool ops (even if not
>    accessing the device) will fail because netif_device_present() returns
>    false
> - ethtool ops may fail because device is not accessible (e.g. because being
>    in D3 in case of a PCI device)
> 
> It may not be desirable that userspace can't use even simple ethtool ops
> that not access the device if interface or link is down. To be more friendly
> to userspace let's ensure that device is runtime-resumed when executing the
> respective ethtool op in kernel.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>   net/ethtool/ioctl.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index baa5d1004..b7ff9abe7 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -23,6 +23,7 @@
>   #include <linux/rtnetlink.h>
>   #include <linux/sched/signal.h>
>   #include <linux/net.h>
> +#include <linux/pm_runtime.h>
>   #include <net/devlink.h>
>   #include <net/xdp_sock_drv.h>
>   #include <net/flow_offload.h>
> @@ -2589,7 +2590,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>   	int rc;
>   	netdev_features_t old_features;
>   
> -	if (!dev || !netif_device_present(dev))
> +	if (!dev)
>   		return -ENODEV;
>   
>   	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
> @@ -2645,10 +2646,18 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>   			return -EPERM;
>   	}
>   
> +	if (dev->dev.parent)
> +		pm_runtime_get_sync(dev->dev.parent);

the PM Runtime should allow to wake up parent when child is resumed if everything is configured properly.

rpm_resume()
...
     if (!parent && dev->parent) {
  --> here

So, hence PM runtime calls are moved to from drivers to net_core wouldn't be more correct approach to
enable PM runtime for netdev->dev and lets PM runtime do the job?

But, to be honest, I'm not sure adding PM runtime manipulation to the net core is a good idea -
at minimum it might be tricky and required very careful approach (especially in err path).
For example, even in this patch you do not check return value of pm_runtime_get_sync() and in
commit bd869245a3dc ("net: core: try to runtime-resume detached device in __dev_open") also actualy.


The TI CPSW driver may also be placed in non reachable state when netdev is closed (and even lose context),
but we do not use netif_device_detach() (so netdev is accessible through netdev_ops/ethtool_ops),
but instead wake up device by runtime PM for allowed operations or just save requested configuration which
is applied at netdev->open() time then.
I feel that using netif_device_detach() in PM runtime sounds like a too heavy approach ;)

huh, see it's merged already, so...

> +
> +	if (!netif_device_present(dev)) {
> +		rc = -ENODEV;
> +		goto out;
> +	}
> +
>   	if (dev->ethtool_ops->begin) {
>   		rc = dev->ethtool_ops->begin(dev);
> -		if (rc  < 0)
> -			return rc;
> +		if (rc < 0)
> +			goto out;
>   	}
>   	old_features = dev->features;
>   
> @@ -2867,6 +2876,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>   
>   	if (old_features != dev->features)
>   		netdev_features_change(dev);
> +out:
> +	if (dev->dev.parent)
> +		pm_runtime_put(dev->dev.parent);
>   
>   	return rc;
>   }
> 

-- 
Best regards,
grygorii