| Message ID | 20190222220758.7117-6-jakub.kicinski@netronome.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | devlink: make ethtool compat reliable | expand |
Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote: >When ethtool is calling into devlink compat code make sure we have >a reference on the netdevice on which the operation was invoked. > >v3: move the hold/lock logic into devlink_compat_* functions (Florian) > >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> >--- > net/core/devlink.c | 34 +++++++++++++++++++++++----------- > net/core/ethtool.c | 13 ++----------- > 2 files changed, 25 insertions(+), 22 deletions(-) > >diff --git a/net/core/devlink.c b/net/core/devlink.c >index a13055160be0..78c6ea1870ca 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev, > { > struct devlink *devlink; > >+ dev_hold(dev); >+ rtnl_unlock(); Ha, I got it now. You rely on dev_hold to make sure the devlink instance does not dissappear. But until this patch, that is not guaranteed (or I'm missing it). >+ > devlink = netdev_to_devlink(dev); >- if (!devlink || !devlink->ops || !devlink->ops->info_get) >- return; >+ if (devlink && devlink->ops && devlink->ops->info_get) { >+ mutex_lock(&devlink->lock); >+ __devlink_compat_running_version(devlink, buf, len); >+ mutex_unlock(&devlink->lock); >+ } > >- mutex_lock(&devlink->lock); >- __devlink_compat_running_version(devlink, buf, len); >- mutex_unlock(&devlink->lock); >+ rtnl_lock(); >+ dev_put(dev); > } > > int devlink_compat_flash_update(struct net_device *dev, const char *file_name) > { > struct devlink *devlink; >- int ret; >+ int ret = -EOPNOTSUPP; >+ >+ dev_hold(dev); >+ rtnl_unlock(); > > devlink = netdev_to_devlink(dev); >- if (!devlink || !devlink->ops || !devlink->ops->flash_update) >- return -EOPNOTSUPP; >+ if (devlink && devlink->ops && devlink->ops->flash_update) { >+ mutex_lock(&devlink->lock); >+ ret = devlink->ops->flash_update(devlink, file_name, >+ NULL, NULL); >+ mutex_unlock(&devlink->lock); >+ } >+ >+ rtnl_lock(); >+ dev_put(dev); > >- mutex_lock(&devlink->lock); >- ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL); >- mutex_unlock(&devlink->lock); > return ret; > } > >diff --git a/net/core/ethtool.c b/net/core/ethtool.c >index 1320e8dce559..47558ffae423 100644 >--- a/net/core/ethtool.c >+++ b/net/core/ethtool.c >@@ -805,11 +805,9 @@ 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]) > devlink_compat_running_version(dev, info.fw_version, > sizeof(info.fw_version)); >- rtnl_lock(); > > if (copy_to_user(useraddr, &info, sizeof(info))) > return -EFAULT; >@@ -2040,15 +2038,8 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, > return -EFAULT; > efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0; > >- if (!dev->ethtool_ops->flash_device) { >- int ret; >- >- rtnl_unlock(); >- ret = devlink_compat_flash_update(dev, efl.data); >- rtnl_lock(); >- >- return ret; >- } >+ if (!dev->ethtool_ops->flash_device) >+ return devlink_compat_flash_update(dev, efl.data); > > return dev->ethtool_ops->flash_device(dev, &efl); > } >-- >2.19.2 >
On Sun, 24 Feb 2019 12:00:03 +0100, Jiri Pirko wrote: > Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote: > >When ethtool is calling into devlink compat code make sure we have > >a reference on the netdevice on which the operation was invoked. > > > >v3: move the hold/lock logic into devlink_compat_* functions (Florian) > > > >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > >--- > > net/core/devlink.c | 34 +++++++++++++++++++++++----------- > > net/core/ethtool.c | 13 ++----------- > > 2 files changed, 25 insertions(+), 22 deletions(-) > > > >diff --git a/net/core/devlink.c b/net/core/devlink.c > >index a13055160be0..78c6ea1870ca 100644 > >--- a/net/core/devlink.c > >+++ b/net/core/devlink.c > >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev, > > { > > struct devlink *devlink; > > > >+ dev_hold(dev); > >+ rtnl_unlock(); > > Ha, I got it now. You rely on dev_hold to make sure the > devlink instance does not dissappear. But until this patch, that is not > guaranteed (or I'm missing it). Yup, I think the expectation that drivers should free netdevs before unregistering devlink holds today. But it may be a source of bugs :S I can add take devlink_mutex, and check the devlink instance is registered. Do you prefer an explicit ->registered field like port has, or can I do this: diff --git a/net/core/devlink.c b/net/core/devlink.c index cefcc0f45d44..be39ad6a4e2e 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -5275,6 +5275,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) return NULL; devlink->ops = ops; devlink_net_set(devlink, &init_net); + INIT_LIST_HEAD(&devlink->list); INIT_LIST_HEAD(&devlink->port_list); INIT_LIST_HEAD(&devlink->sb_list); INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list); @@ -5303,6 +5304,11 @@ int devlink_register(struct devlink *devlink, struct device *dev) } EXPORT_SYMBOL_GPL(devlink_register); +static bool devlink_is_registered(struct devlink *devlink) +{ + return list_empty(&devlink->list); +} + /** * devlink_unregister - Unregister devlink instance * @@ -5312,7 +5318,7 @@ void devlink_unregister(struct devlink *devlink) { mutex_lock(&devlink_mutex); devlink_notify(devlink, DEVLINK_CMD_DEL); - list_del(&devlink->list); + list_del_init(&devlink->list); mutex_unlock(&devlink_mutex); } EXPORT_SYMBOL_GPL(devlink_unregister); ?
On Mon, 25 Feb 2019 10:31:27 -0800, Jakub Kicinski wrote: > +static bool devlink_is_registered(struct devlink *devlink) > +{ > + return list_empty(&devlink->list); > +} Nevermind, this won't really help the drivers that much, those registering devlink after netdevs will have to take care of the potential race by marking the devlink instance as dead in their priv data structure from the thread doing the devlink_unregister, using their own locking.
Mon, Feb 25, 2019 at 07:31:27PM CET, jakub.kicinski@netronome.com wrote: >On Sun, 24 Feb 2019 12:00:03 +0100, Jiri Pirko wrote: >> Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote: >> >When ethtool is calling into devlink compat code make sure we have >> >a reference on the netdevice on which the operation was invoked. >> > >> >v3: move the hold/lock logic into devlink_compat_* functions (Florian) >> > >> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> >> >--- >> > net/core/devlink.c | 34 +++++++++++++++++++++++----------- >> > net/core/ethtool.c | 13 ++----------- >> > 2 files changed, 25 insertions(+), 22 deletions(-) >> > >> >diff --git a/net/core/devlink.c b/net/core/devlink.c >> >index a13055160be0..78c6ea1870ca 100644 >> >--- a/net/core/devlink.c >> >+++ b/net/core/devlink.c >> >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev, >> > { >> > struct devlink *devlink; >> > >> >+ dev_hold(dev); >> >+ rtnl_unlock(); >> >> Ha, I got it now. You rely on dev_hold to make sure the >> devlink instance does not dissappear. But until this patch, that is not >> guaranteed (or I'm missing it). > >Yup, I think the expectation that drivers should free netdevs before >unregistering devlink holds today. But it may be a source of bugs :S Sure. > >I can add take devlink_mutex, and check the devlink instance is >registered. Do you prefer an explicit ->registered field like port has, >or can I do this: Yeah, let's add WARN_ON. > >diff --git a/net/core/devlink.c b/net/core/devlink.c >index cefcc0f45d44..be39ad6a4e2e 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -5275,6 +5275,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size) > return NULL; > devlink->ops = ops; > devlink_net_set(devlink, &init_net); >+ INIT_LIST_HEAD(&devlink->list); > INIT_LIST_HEAD(&devlink->port_list); > INIT_LIST_HEAD(&devlink->sb_list); > INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list); >@@ -5303,6 +5304,11 @@ int devlink_register(struct devlink *devlink, struct device *dev) > } > EXPORT_SYMBOL_GPL(devlink_register); > >+static bool devlink_is_registered(struct devlink *devlink) >+{ >+ return list_empty(&devlink->list); >+} >+ > /** > * devlink_unregister - Unregister devlink instance > * >@@ -5312,7 +5318,7 @@ void devlink_unregister(struct devlink *devlink) > { > mutex_lock(&devlink_mutex); > devlink_notify(devlink, DEVLINK_CMD_DEL); >- list_del(&devlink->list); >+ list_del_init(&devlink->list); > mutex_unlock(&devlink_mutex); > } > EXPORT_SYMBOL_GPL(devlink_unregister); > >?
diff --git a/net/core/devlink.c b/net/core/devlink.c index a13055160be0..78c6ea1870ca 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev, { struct devlink *devlink; + dev_hold(dev); + rtnl_unlock(); + devlink = netdev_to_devlink(dev); - if (!devlink || !devlink->ops || !devlink->ops->info_get) - return; + if (devlink && devlink->ops && devlink->ops->info_get) { + mutex_lock(&devlink->lock); + __devlink_compat_running_version(devlink, buf, len); + mutex_unlock(&devlink->lock); + } - mutex_lock(&devlink->lock); - __devlink_compat_running_version(devlink, buf, len); - mutex_unlock(&devlink->lock); + rtnl_lock(); + dev_put(dev); } int devlink_compat_flash_update(struct net_device *dev, const char *file_name) { struct devlink *devlink; - int ret; + int ret = -EOPNOTSUPP; + + dev_hold(dev); + rtnl_unlock(); devlink = netdev_to_devlink(dev); - if (!devlink || !devlink->ops || !devlink->ops->flash_update) - return -EOPNOTSUPP; + if (devlink && devlink->ops && devlink->ops->flash_update) { + mutex_lock(&devlink->lock); + ret = devlink->ops->flash_update(devlink, file_name, + NULL, NULL); + mutex_unlock(&devlink->lock); + } + + rtnl_lock(); + dev_put(dev); - mutex_lock(&devlink->lock); - ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL); - mutex_unlock(&devlink->lock); return ret; } diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 1320e8dce559..47558ffae423 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -805,11 +805,9 @@ 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]) devlink_compat_running_version(dev, info.fw_version, sizeof(info.fw_version)); - rtnl_lock(); if (copy_to_user(useraddr, &info, sizeof(info))) return -EFAULT; @@ -2040,15 +2038,8 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, return -EFAULT; efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0; - if (!dev->ethtool_ops->flash_device) { - int ret; - - rtnl_unlock(); - ret = devlink_compat_flash_update(dev, efl.data); - rtnl_lock(); - - return ret; - } + if (!dev->ethtool_ops->flash_device) + return devlink_compat_flash_update(dev, efl.data); return dev->ethtool_ops->flash_device(dev, &efl); }
When ethtool is calling into devlink compat code make sure we have a reference on the netdevice on which the operation was invoked. v3: move the hold/lock logic into devlink_compat_* functions (Florian) Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- net/core/devlink.c | 34 +++++++++++++++++++++++----------- net/core/ethtool.c | 13 ++----------- 2 files changed, 25 insertions(+), 22 deletions(-)