| Message ID | 05bae6c6-502e-4715-1283-fc4135702515@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ethtool: runtime-resume netdev parent before ethtool ops | expand |
On 01.08.21 13:41, 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@gmail.com> > --- > net/ethtool/netlink.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > [...] > > void ethnl_ops_complete(struct net_device *dev) > { > if (dev && dev->ethtool_ops->complete) > dev->ethtool_ops->complete(dev); > + > + if (dev->dev.parent) > + pm_runtime_put(dev->dev.parent); > } > > /** > Hello Heiner, Coverity complains that we checked dev != NULL earlier but now unconditionally dereference it: *** CID 1506213: Null pointer dereferences (FORWARD_NULL) /net/ethtool/netlink.c: 67 in ethnl_ops_complete() 61 62 void ethnl_ops_complete(struct net_device *dev) 63 { 64 if (dev && dev->ethtool_ops->complete) 65 dev->ethtool_ops->complete(dev); 66 >>> CID 1506213: Null pointer dereferences (FORWARD_NULL) >>> Dereferencing null pointer "dev". 67 if (dev->dev.parent) 68 pm_runtime_put(dev->dev.parent); 69 } 70 71 /** 72 * ethnl_parse_header_dev_get() - parse request header
On 05.08.2021 13:51, Julian Wiedmann wrote: > On 01.08.21 13:41, 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@gmail.com> >> --- >> net/ethtool/netlink.c | 31 +++++++++++++++++++++++++------ >> 1 file changed, 25 insertions(+), 6 deletions(-) >> > > [...] > >> >> void ethnl_ops_complete(struct net_device *dev) >> { >> if (dev && dev->ethtool_ops->complete) >> dev->ethtool_ops->complete(dev); >> + >> + if (dev->dev.parent) >> + pm_runtime_put(dev->dev.parent); >> } >> >> /** >> > > Hello Heiner, > > Coverity complains that we checked dev != NULL earlier but now > unconditionally dereference it: > Thanks for the hint. I wonder whether we have any valid case where dev could be NULL. There are several places where dev is dereferenced after the call to ethnl_ops_begin(). Just one example: linkmodes_prepare_data() Only ethnl_request_ops where allow_nodev_do is true is ethnl_strset_request_ops. However in strset_prepare_data() ethnl_ops_begin() is called only if dev isn't NULL. Supposedly we should return an error from ethnl_ops_begin() if dev is NULL. > > *** CID 1506213: Null pointer dereferences (FORWARD_NULL) > /net/ethtool/netlink.c: 67 in ethnl_ops_complete() > 61 > 62 void ethnl_ops_complete(struct net_device *dev) > 63 { > 64 if (dev && dev->ethtool_ops->complete) > 65 dev->ethtool_ops->complete(dev); > 66 >>>> CID 1506213: Null pointer dereferences (FORWARD_NULL) >>>> Dereferencing null pointer "dev". > 67 if (dev->dev.parent) > 68 pm_runtime_put(dev->dev.parent); > 69 } > 70 > 71 /** > 72 * ethnl_parse_header_dev_get() - parse request header >
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index e628d17f5..417aaf9ca 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -2,6 +2,7 @@ #include <net/sock.h> #include <linux/ethtool_netlink.h> +#include <linux/pm_runtime.h> #include "netlink.h" static struct genl_family ethtool_genl_family; @@ -31,22 +32,40 @@ const struct nla_policy ethnl_header_policy_stats[] = { int ethnl_ops_begin(struct net_device *dev) { + int ret; + if (!dev) return 0; - if (!netif_device_present(dev)) - return -ENODEV; + if (dev->dev.parent) + pm_runtime_get_sync(dev->dev.parent); - if (dev->ethtool_ops->begin) - return dev->ethtool_ops->begin(dev); - else - return 0; + if (!netif_device_present(dev)) { + ret = -ENODEV; + goto err; + } + + if (dev->ethtool_ops->begin) { + ret = dev->ethtool_ops->begin(dev); + if (ret) + goto err; + } + + return 0; +err: + if (dev->dev.parent) + pm_runtime_put(dev->dev.parent); + + return ret; } void ethnl_ops_complete(struct net_device *dev) { if (dev && dev->ethtool_ops->complete) dev->ethtool_ops->complete(dev); + + if (dev->dev.parent) + pm_runtime_put(dev->dev.parent); } /**
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@gmail.com> --- net/ethtool/netlink.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)