[net-next,v3,5/6] devlink: hold a reference to the netdevice around ethtool compat
diff mbox series

Message ID 20190222220758.7117-6-jakub.kicinski@netronome.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • devlink: make ethtool compat reliable
Related show

Commit Message

Jakub Kicinski Feb. 22, 2019, 10:07 p.m. UTC
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(-)

Comments

Jiri Pirko Feb. 24, 2019, 11 a.m. UTC | #1
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
>
Jakub Kicinski Feb. 25, 2019, 6:31 p.m. UTC | #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);

?
Jakub Kicinski Feb. 26, 2019, 3:07 a.m. UTC | #3
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.
Jiri Pirko Feb. 26, 2019, 6:50 a.m. UTC | #4
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);
>
>?

Patch
diff mbox series

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);
 }