| Message ID | 20190222195410.9494-3-jakub.kicinski@netronome.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | devlink: make ethtool compat reliable | expand |
On 2/22/19 11:54 AM, Jakub Kicinski wrote: > When calling into devlink compat code make sure we have a reference > on the netdevice on which the operation was invoked. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > net/core/ethtool.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 1320e8dce559..6832476dfcaf 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev, > if (ops->get_eeprom_len) > info.eedump_len = ops->get_eeprom_len(dev); > > - rtnl_unlock(); > - if (!info.fw_version[0]) > + if (!info.fw_version[0]) { > + dev_hold(dev); > + rtnl_unlock(); > devlink_compat_running_version(dev, info.fw_version, > sizeof(info.fw_version)); > - rtnl_lock(); > + rtnl_lock(); > + dev_put(dev); > + } Would it make sense to make the locking and reference holding implicit within the compat versions of devlink_* because they use a net_device -> devlink object manipulation as opposed to doing in the caller. We are more or less guaranteed that the compatibility layer is used from within ethtool.
On Fri, 22 Feb 2019 12:09:32 -0800, Florian Fainelli wrote: > On 2/22/19 11:54 AM, Jakub Kicinski wrote: > > When calling into devlink compat code make sure we have a reference > > on the netdevice on which the operation was invoked. > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > --- > > net/core/ethtool.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > > index 1320e8dce559..6832476dfcaf 100644 > > --- a/net/core/ethtool.c > > +++ b/net/core/ethtool.c > > @@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev, > > if (ops->get_eeprom_len) > > info.eedump_len = ops->get_eeprom_len(dev); > > > > - rtnl_unlock(); > > - if (!info.fw_version[0]) > > + if (!info.fw_version[0]) { > > + dev_hold(dev); > > + rtnl_unlock(); > > devlink_compat_running_version(dev, info.fw_version, > > sizeof(info.fw_version)); > > - rtnl_lock(); > > + rtnl_lock(); > > + dev_put(dev); > > + } > > Would it make sense to make the locking and reference holding implicit > within the compat versions of devlink_* because they use a net_device -> > devlink object manipulation as opposed to doing in the caller. We are > more or less guaranteed that the compatibility layer is used from within > ethtool. I guess we can, in general it feels wrong to call some devlink_ function with rtnl_lock held, but I guess devlink_compat_ can be special in this regard. v3 coming up.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 1320e8dce559..6832476dfcaf 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -805,11 +805,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev, if (ops->get_eeprom_len) info.eedump_len = ops->get_eeprom_len(dev); - rtnl_unlock(); - if (!info.fw_version[0]) + if (!info.fw_version[0]) { + dev_hold(dev); + rtnl_unlock(); devlink_compat_running_version(dev, info.fw_version, sizeof(info.fw_version)); - rtnl_lock(); + rtnl_lock(); + dev_put(dev); + } if (copy_to_user(useraddr, &info, sizeof(info))) return -EFAULT; @@ -2043,9 +2046,11 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, if (!dev->ethtool_ops->flash_device) { int ret; + dev_hold(dev); rtnl_unlock(); ret = devlink_compat_flash_update(dev, efl.data); rtnl_lock(); + dev_put(dev); return ret; }
When calling into devlink compat code make sure we have a reference on the netdevice on which the operation was invoked. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- net/core/ethtool.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)