[PATCH net-next v2 0/4] improve ethtool/rtnl vs devlink locking

[PATCH net-next v2 0/4] improve ethtool/rtnl vs devlink locking

[Jakub Kicinski]

During ethtool netlink development we decided to move some of
the commmands to devlink. Since we don't want drivers to implement
both devlink and ethtool version of the commands ethtool ioctl
falls back to calling devlink. Unfortunately devlink locks must
be taken before rtnl_lock. This results in a questionable
dev_hold() / rtnl_unlock() / devlink / rtnl_lock() / dev_put()
pattern.

This method "works" but it working depends on drivers in question
not doing much in ethtool_ops->begin / complete, and on the netdev
not having needs_free_netdev set.

Since commit 437ebfd90a25 ("devlink: Count struct devlink consumers")
we can hold a reference on a devlink instance and prevent it from
going away (sort of like netdev with dev_hold()). We can use this
to create a more natural reference nesting where we get a ref on
the devlink instance and make the devlink call entirely outside
of the rtnl_lock section.

v2: remember to update the static inline stubs as well (kbuild bot)

Jakub Kicinski (4):
  ethtool: push the rtnl_lock into dev_ethtool()
  ethtool: handle info/flash data copying outside rtnl_lock
  devlink: expose get/put functions
  ethtool: don't drop the rtnl_lock half way thru the ioctl

 include/net/devlink.h |  20 ++++--
 net/core/dev_ioctl.c  |   2 -
 net/core/devlink.c    |  53 ++++-----------
 net/ethtool/ioctl.c   | 148 ++++++++++++++++++++++++++++++------------
 4 files changed, 136 insertions(+), 87 deletions(-)


base-commit: ae0393500e3b0139210749d52d22b29002c20e16
-- 
2.31.1

[PATCH net-next v2 1/4] ethtool: push the rtnl_lock into dev_ethtool()

[Jakub Kicinski]

Don't take the lock in net/core/dev_ioctl.c,
we'll have things to do outside rtnl_lock soon.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev_ioctl.c |  2 --
 net/ethtool/ioctl.c  | 14 +++++++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 0e87237fd871..cbab5fec64b1 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -518,9 +518,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 
 	case SIOCETHTOOL:
 		dev_load(net, ifr->ifr_name);
-		rtnl_lock();
 		ret = dev_ethtool(net, ifr, data);
-		rtnl_unlock();
 		if (colon)
 			*colon = ':';
 		return ret;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 44430b6ab843..52bfc5b82ec3 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2700,7 +2700,8 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
-int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
+static int
+__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 {
 	struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
 	u32 ethcmd, sub_cmd;
@@ -3000,6 +3001,17 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 	return rc;
 }
 
+int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
+{
+	int rc;
+
+	rtnl_lock();
+	rc = __dev_ethtool(net, ifr, useraddr);
+	rtnl_unlock();
+
+	return rc;
+}
+
 struct ethtool_rx_flow_key {
 	struct flow_dissector_key_basic			basic;
 	union {
-- 
2.31.1

[PATCH net-next v2 2/4] ethtool: handle info/flash data copying outside rtnl_lock

[Jakub Kicinski]

We need to increase the lifetime of the data for .get_info
and .flash_update beyond their handlers inside rtnl_lock.

Allocate a union on the heap and use it instead.

Note that we now copy the ethcmd before we lookup dev,
hopefully there is no crazy user space depending on error
codes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/ioctl.c | 110 +++++++++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 41 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 52bfc5b82ec3..1980e37b6472 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -32,6 +32,14 @@
 #include <generated/utsrelease.h>
 #include "common.h"
 
+/* State held across locks and calls for commands which have devlink fallback */
+struct ethtool_devlink_compat {
+	union {
+		struct ethtool_flash efl;
+		struct ethtool_drvinfo info;
+	};
+};
+
 /*
  * Some useful ethtool_ops methods that're device independent.
  * If we find that all drivers want to do the same thing here,
@@ -697,22 +705,20 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
-static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
-						  void __user *useraddr)
+static int
+ethtool_get_drvinfo(struct net_device *dev, struct ethtool_devlink_compat *rsp)
 {
-	struct ethtool_drvinfo info;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 
-	memset(&info, 0, sizeof(info));
-	info.cmd = ETHTOOL_GDRVINFO;
-	strlcpy(info.version, UTS_RELEASE, sizeof(info.version));
+	rsp->info.cmd = ETHTOOL_GDRVINFO;
+	strlcpy(rsp->info.version, UTS_RELEASE, sizeof(rsp->info.version));
 	if (ops->get_drvinfo) {
-		ops->get_drvinfo(dev, &info);
+		ops->get_drvinfo(dev, &rsp->info);
 	} else if (dev->dev.parent && dev->dev.parent->driver) {
-		strlcpy(info.bus_info, dev_name(dev->dev.parent),
-			sizeof(info.bus_info));
-		strlcpy(info.driver, dev->dev.parent->driver->name,
-			sizeof(info.driver));
+		strlcpy(rsp->info.bus_info, dev_name(dev->dev.parent),
+			sizeof(rsp->info.bus_info));
+		strlcpy(rsp->info.driver, dev->dev.parent->driver->name,
+			sizeof(rsp->info.driver));
 	} else {
 		return -EOPNOTSUPP;
 	}
@@ -726,30 +732,27 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
 
 		rc = ops->get_sset_count(dev, ETH_SS_TEST);
 		if (rc >= 0)
-			info.testinfo_len = rc;
+			rsp->info.testinfo_len = rc;
 		rc = ops->get_sset_count(dev, ETH_SS_STATS);
 		if (rc >= 0)
-			info.n_stats = rc;
+			rsp->info.n_stats = rc;
 		rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
 		if (rc >= 0)
-			info.n_priv_flags = rc;
+			rsp->info.n_priv_flags = rc;
 	}
 	if (ops->get_regs_len) {
 		int ret = ops->get_regs_len(dev);
 
 		if (ret > 0)
-			info.regdump_len = ret;
+			rsp->info.regdump_len = ret;
 	}
 
 	if (ops->get_eeprom_len)
-		info.eedump_len = ops->get_eeprom_len(dev);
-
-	if (!info.fw_version[0])
-		devlink_compat_running_version(dev, info.fw_version,
-					       sizeof(info.fw_version));
+		rsp->info.eedump_len = ops->get_eeprom_len(dev);
 
-	if (copy_to_user(useraddr, &info, sizeof(info)))
-		return -EFAULT;
+	if (!rsp->info.fw_version[0])
+		devlink_compat_running_version(dev, rsp->info.fw_version,
+					       sizeof(rsp->info.fw_version));
 	return 0;
 }
 
@@ -2178,19 +2181,13 @@ static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
 	return actor(dev, edata.data);
 }
 
-static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
-						   char __user *useraddr)
+static int
+ethtool_flash_device(struct net_device *dev, struct ethtool_devlink_compat *req)
 {
-	struct ethtool_flash efl;
-
-	if (copy_from_user(&efl, useraddr, sizeof(efl)))
-		return -EFAULT;
-	efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
-
 	if (!dev->ethtool_ops->flash_device)
-		return devlink_compat_flash_update(dev, efl.data);
+		return devlink_compat_flash_update(dev, req->efl.data);
 
-	return dev->ethtool_ops->flash_device(dev, &efl);
+	return dev->ethtool_ops->flash_device(dev, &req->efl);
 }
 
 static int ethtool_set_dump(struct net_device *dev,
@@ -2701,19 +2698,18 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 static int
-__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
+__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
+	      u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
 {
-	struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
-	u32 ethcmd, sub_cmd;
+	struct net_device *dev;
+	u32 sub_cmd;
 	int rc;
 	netdev_features_t old_features;
 
+	dev = __dev_get_by_name(net, ifr->ifr_name);
 	if (!dev)
 		return -ENODEV;
 
-	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
-		return -EFAULT;
-
 	if (ethcmd == ETHTOOL_PERQUEUE) {
 		if (copy_from_user(&sub_cmd, useraddr + sizeof(ethcmd), sizeof(sub_cmd)))
 			return -EFAULT;
@@ -2787,7 +2783,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		rc = ethtool_set_settings(dev, useraddr);
 		break;
 	case ETHTOOL_GDRVINFO:
-		rc = ethtool_get_drvinfo(dev, useraddr);
+		rc = ethtool_get_drvinfo(dev, devlink_state);
 		break;
 	case ETHTOOL_GREGS:
 		rc = ethtool_get_regs(dev, useraddr);
@@ -2889,7 +2885,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		rc = ethtool_set_rxnfc(dev, ethcmd, useraddr);
 		break;
 	case ETHTOOL_FLASHDEV:
-		rc = ethtool_flash_device(dev, useraddr);
+		rc = ethtool_flash_device(dev, devlink_state);
 		break;
 	case ETHTOOL_RESET:
 		rc = ethtool_reset(dev, useraddr);
@@ -3003,12 +2999,44 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 
 int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 {
+	struct ethtool_devlink_compat *state;
+	u32 ethcmd;
 	int rc;
 
+	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
+		return -EFAULT;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	switch (ethcmd) {
+	case ETHTOOL_FLASHDEV:
+		if (copy_from_user(&state->efl, useraddr, sizeof(state->efl))) {
+			rc = -EFAULT;
+			goto exit_free;
+		}
+		state->efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
+		break;
+	}
+
 	rtnl_lock();
-	rc = __dev_ethtool(net, ifr, useraddr);
+	rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
 	rtnl_unlock();
+	if (rc)
+		goto exit_free;
+
+	switch (ethcmd) {
+	case ETHTOOL_GDRVINFO:
+		if (copy_to_user(useraddr, &state->info, sizeof(state->info))) {
+			rc = -EFAULT;
+			goto exit_free;
+		}
+		break;
+	}
 
+exit_free:
+	kfree(state);
 	return rc;
 }
 
-- 
2.31.1

[PATCH net-next v2 3/4] devlink: expose get/put functions

[Jakub Kicinski]

Allow those who hold implicit reference on a devlink instance
to try to take a full ref on it. This will be used from netdev
code which has an implicit ref because of driver call ordering.

Note that after recent changes devlink_unregister() may happen
before netdev unregister, but devlink_free() should still happen
after, so we are safe to try, but we can't just refcount_inc()
and assume it's not zero.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/devlink.h | 12 ++++++++++++
 net/core/devlink.c    |  8 +++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 1b1317d378de..991ce48f77ca 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1726,6 +1726,9 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 
 #if IS_ENABLED(CONFIG_NET_DEVLINK)
 
+struct devlink *__must_check devlink_try_get(struct devlink *devlink);
+void devlink_put(struct devlink *devlink);
+
 void devlink_compat_running_version(struct net_device *dev,
 				    char *buf, size_t len);
 int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
@@ -1736,6 +1739,15 @@ int devlink_compat_switch_id_get(struct net_device *dev,
 
 #else
 
+static inline struct devlink *devlink_try_get(struct devlink *devlink)
+{
+	return NULL;
+}
+
+static inline void devlink_put(struct devlink *devlink)
+{
+}
+
 static inline void
 devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
 {
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 2d8abe88c673..100d87fd3f65 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -182,15 +182,17 @@ struct net *devlink_net(const struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devlink_net);
 
-static void devlink_put(struct devlink *devlink)
+void devlink_put(struct devlink *devlink)
 {
 	if (refcount_dec_and_test(&devlink->refcount))
 		complete(&devlink->comp);
 }
 
-static bool __must_check devlink_try_get(struct devlink *devlink)
+struct devlink *__must_check devlink_try_get(struct devlink *devlink)
 {
-	return refcount_inc_not_zero(&devlink->refcount);
+	if (refcount_inc_not_zero(&devlink->refcount))
+		return devlink;
+	return NULL;
 }
 
 static struct devlink *devlink_get_from_attrs(struct net *net,
-- 
2.31.1

[PATCH net-next v2 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl

[Jakub Kicinski]

devlink compat code needs to drop rtnl_lock to take
devlink->lock to ensure correct lock ordering.

This is problematic because we're not strictly guaranteed
that the netdev will not disappear after we re-lock.
It may open a possibility of nested ->begin / ->complete
calls.

Instead of calling into devlink under rtnl_lock take
a ref on the devlink instance and make the call after
we've dropped rtnl_lock.

We (continue to) assume that netdevs have an implicit
reference on the devlink returned from ndo_get_devlink_port

Note that ndo_get_devlink_port will now get called
under rtnl_lock. That should be fine since none of
the drivers seem to be taking serious locks inside
ndo_get_devlink_port.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/devlink.h |  8 ++++----
 net/core/devlink.c    | 45 +++++++------------------------------------
 net/ethtool/ioctl.c   | 36 ++++++++++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 46 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 991ce48f77ca..aab3d007c577 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1729,9 +1729,9 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 struct devlink *__must_check devlink_try_get(struct devlink *devlink);
 void devlink_put(struct devlink *devlink);
 
-void devlink_compat_running_version(struct net_device *dev,
+void devlink_compat_running_version(struct devlink *devlink,
 				    char *buf, size_t len);
-int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
+int devlink_compat_flash_update(struct devlink *devlink, const char *file_name);
 int devlink_compat_phys_port_name_get(struct net_device *dev,
 				      char *name, size_t len);
 int devlink_compat_switch_id_get(struct net_device *dev,
@@ -1749,12 +1749,12 @@ static inline void devlink_put(struct devlink *devlink)
 }
 
 static inline void
-devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
+devlink_compat_running_version(struct devlink *devlink, char *buf, size_t len)
 {
 }
 
 static inline int
-devlink_compat_flash_update(struct net_device *dev, const char *file_name)
+devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 100d87fd3f65..6b5ee862429e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -11283,55 +11283,28 @@ static struct devlink_port *netdev_to_devlink_port(struct net_device *dev)
 	return dev->netdev_ops->ndo_get_devlink_port(dev);
 }
 
-static struct devlink *netdev_to_devlink(struct net_device *dev)
-{
-	struct devlink_port *devlink_port = netdev_to_devlink_port(dev);
-
-	if (!devlink_port)
-		return NULL;
-
-	return devlink_port->devlink;
-}
-
-void devlink_compat_running_version(struct net_device *dev,
+void devlink_compat_running_version(struct devlink *devlink,
 				    char *buf, size_t len)
 {
-	struct devlink *devlink;
-
-	dev_hold(dev);
-	rtnl_unlock();
-
-	devlink = netdev_to_devlink(dev);
-	if (!devlink || !devlink->ops->info_get)
-		goto out;
+	if (!devlink->ops->info_get)
+		return;
 
 	mutex_lock(&devlink->lock);
 	__devlink_compat_running_version(devlink, buf, len);
 	mutex_unlock(&devlink->lock);
-
-out:
-	rtnl_lock();
-	dev_put(dev);
 }
 
-int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
+int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
 {
 	struct devlink_flash_update_params params = {};
-	struct devlink *devlink;
 	int ret;
 
-	dev_hold(dev);
-	rtnl_unlock();
-
-	devlink = netdev_to_devlink(dev);
-	if (!devlink || !devlink->ops->flash_update) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
+	if (!devlink->ops->flash_update)
+		return -EOPNOTSUPP;
 
 	ret = request_firmware(&params.fw, file_name, devlink->dev);
 	if (ret)
-		goto out;
+		return ret;
 
 	mutex_lock(&devlink->lock);
 	devlink_flash_update_begin_notify(devlink);
@@ -11341,10 +11314,6 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 
 	release_firmware(params.fw);
 
-out:
-	rtnl_lock();
-	dev_put(dev);
-
 	return ret;
 }
 
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 1980e37b6472..65e9bc1058b5 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -34,12 +34,27 @@
 
 /* State held across locks and calls for commands which have devlink fallback */
 struct ethtool_devlink_compat {
+	struct devlink *devlink;
 	union {
 		struct ethtool_flash efl;
 		struct ethtool_drvinfo info;
 	};
 };
 
+static struct devlink *netdev_to_devlink_get(struct net_device *dev)
+{
+	struct devlink_port *devlink_port;
+
+	if (!dev->netdev_ops->ndo_get_devlink_port)
+		return NULL;
+
+	devlink_port = dev->netdev_ops->ndo_get_devlink_port(dev);
+	if (!devlink_port)
+		return NULL;
+
+	return devlink_try_get(devlink_port->devlink);
+}
+
 /*
  * Some useful ethtool_ops methods that're device independent.
  * If we find that all drivers want to do the same thing here,
@@ -751,8 +766,8 @@ ethtool_get_drvinfo(struct net_device *dev, struct ethtool_devlink_compat *rsp)
 		rsp->info.eedump_len = ops->get_eeprom_len(dev);
 
 	if (!rsp->info.fw_version[0])
-		devlink_compat_running_version(dev, rsp->info.fw_version,
-					       sizeof(rsp->info.fw_version));
+		rsp->devlink = netdev_to_devlink_get(dev);
+
 	return 0;
 }
 
@@ -2184,8 +2199,10 @@ static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
 static int
 ethtool_flash_device(struct net_device *dev, struct ethtool_devlink_compat *req)
 {
-	if (!dev->ethtool_ops->flash_device)
-		return devlink_compat_flash_update(dev, req->efl.data);
+	if (!dev->ethtool_ops->flash_device) {
+		req->devlink = netdev_to_devlink_get(dev);
+		return 0;
+	}
 
 	return dev->ethtool_ops->flash_device(dev, &req->efl);
 }
@@ -3027,7 +3044,16 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		goto exit_free;
 
 	switch (ethcmd) {
+	case ETHTOOL_FLASHDEV:
+		if (state->devlink)
+			rc = devlink_compat_flash_update(state->devlink,
+							 state->efl.data);
+		break;
 	case ETHTOOL_GDRVINFO:
+		if (state->devlink)
+			devlink_compat_running_version(state->devlink,
+						       state->info.fw_version,
+						       sizeof(state->info.fw_version));
 		if (copy_to_user(useraddr, &state->info, sizeof(state->info))) {
 			rc = -EFAULT;
 			goto exit_free;
@@ -3036,6 +3062,8 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 	}
 
 exit_free:
+	if (state->devlink)
+		devlink_put(state->devlink);
 	kfree(state);
 	return rc;
 }
-- 
2.31.1

Re: [PATCH net-next v2 1/4] ethtool: push the rtnl_lock into dev_ethtool()

[Leon Romanovsky]

On Sat, Oct 30, 2021 at 10:18:48AM -0700, Jakub Kicinski wrote:
> Don't take the lock in net/core/dev_ioctl.c,
> we'll have things to do outside rtnl_lock soon.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/dev_ioctl.c |  2 --
>  net/ethtool/ioctl.c  | 14 +++++++++++++-
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next v2 2/4] ethtool: handle info/flash data copying outside rtnl_lock

[Leon Romanovsky]

On Sat, Oct 30, 2021 at 10:18:49AM -0700, Jakub Kicinski wrote:
> We need to increase the lifetime of the data for .get_info
> and .flash_update beyond their handlers inside rtnl_lock.
> 
> Allocate a union on the heap and use it instead.
> 
> Note that we now copy the ethcmd before we lookup dev,
> hopefully there is no crazy user space depending on error
> codes.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/ethtool/ioctl.c | 110 +++++++++++++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 41 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next v2 3/4] devlink: expose get/put functions

[Leon Romanovsky]

On Sat, Oct 30, 2021 at 10:18:50AM -0700, Jakub Kicinski wrote:
> Allow those who hold implicit reference on a devlink instance
> to try to take a full ref on it. This will be used from netdev
> code which has an implicit ref because of driver call ordering.
> 
> Note that after recent changes devlink_unregister() may happen
> before netdev unregister, but devlink_free() should still happen
> after, so we are safe to try, but we can't just refcount_inc()
> and assume it's not zero.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/net/devlink.h | 12 ++++++++++++
>  net/core/devlink.c    |  8 +++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)

I really like this series, but your latest netdevsim RFC made me worry.

It is important to make sure that these devlink_put() and devlink_get()
calls will be out-of-reach from the drivers. Only core code should use
them.

Can you please add it as a comment above these functions?
At least for now, till we discuss your RFC.

Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Thanks

> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 1b1317d378de..991ce48f77ca 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1726,6 +1726,9 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>  
>  #if IS_ENABLED(CONFIG_NET_DEVLINK)
>  
> +struct devlink *__must_check devlink_try_get(struct devlink *devlink);
> +void devlink_put(struct devlink *devlink);
> +
>  void devlink_compat_running_version(struct net_device *dev,
>  				    char *buf, size_t len);
>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
> @@ -1736,6 +1739,15 @@ int devlink_compat_switch_id_get(struct net_device *dev,
>  
>  #else
>  
> +static inline struct devlink *devlink_try_get(struct devlink *devlink)
> +{
> +	return NULL;
> +}
> +
> +static inline void devlink_put(struct devlink *devlink)
> +{
> +}
> +
>  static inline void
>  devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
>  {
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 2d8abe88c673..100d87fd3f65 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -182,15 +182,17 @@ struct net *devlink_net(const struct devlink *devlink)
>  }
>  EXPORT_SYMBOL_GPL(devlink_net);
>  
> -static void devlink_put(struct devlink *devlink)
> +void devlink_put(struct devlink *devlink)
>  {
>  	if (refcount_dec_and_test(&devlink->refcount))
>  		complete(&devlink->comp);
>  }
>  
> -static bool __must_check devlink_try_get(struct devlink *devlink)
> +struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>  {
> -	return refcount_inc_not_zero(&devlink->refcount);
> +	if (refcount_inc_not_zero(&devlink->refcount))
> +		return devlink;
> +	return NULL;
>  }
>  
>  static struct devlink *devlink_get_from_attrs(struct net *net,
> -- 
> 2.31.1
> 

Re: [PATCH net-next v2 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl

[Leon Romanovsky]

On Sat, Oct 30, 2021 at 10:18:51AM -0700, Jakub Kicinski wrote:
> devlink compat code needs to drop rtnl_lock to take
> devlink->lock to ensure correct lock ordering.
> 
> This is problematic because we're not strictly guaranteed
> that the netdev will not disappear after we re-lock.
> It may open a possibility of nested ->begin / ->complete
> calls.
> 
> Instead of calling into devlink under rtnl_lock take
> a ref on the devlink instance and make the call after
> we've dropped rtnl_lock.
> 
> We (continue to) assume that netdevs have an implicit
> reference on the devlink returned from ndo_get_devlink_port
> 
> Note that ndo_get_devlink_port will now get called
> under rtnl_lock. That should be fine since none of
> the drivers seem to be taking serious locks inside
> ndo_get_devlink_port.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/net/devlink.h |  8 ++++----
>  net/core/devlink.c    | 45 +++++++------------------------------------
>  net/ethtool/ioctl.c   | 36 ++++++++++++++++++++++++++++++----
>  3 files changed, 43 insertions(+), 46 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next v2 0/4] improve ethtool/rtnl vs devlink locking

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Sat, 30 Oct 2021 10:18:47 -0700 you wrote:
> During ethtool netlink development we decided to move some of
> the commmands to devlink. Since we don't want drivers to implement
> both devlink and ethtool version of the commands ethtool ioctl
> falls back to calling devlink. Unfortunately devlink locks must
> be taken before rtnl_lock. This results in a questionable
> dev_hold() / rtnl_unlock() / devlink / rtnl_lock() / dev_put()
> pattern.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/4] ethtool: push the rtnl_lock into dev_ethtool()
    https://git.kernel.org/netdev/net-next/c/f49deaa64af1
  - [net-next,v2,2/4] ethtool: handle info/flash data copying outside rtnl_lock
    https://git.kernel.org/netdev/net-next/c/095cfcfe13e5
  - [net-next,v2,3/4] devlink: expose get/put functions
    https://git.kernel.org/netdev/net-next/c/46db1b77cd4f
  - [net-next,v2,4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl
    https://git.kernel.org/netdev/net-next/c/1af0a0948e28

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2 3/4] devlink: expose get/put functions

[Jakub Kicinski]

On Sun, 31 Oct 2021 08:29:20 +0200 Leon Romanovsky wrote:
> I really like this series, but your latest netdevsim RFC made me worry.
> 
> It is important to make sure that these devlink_put() and devlink_get()
> calls will be out-of-reach from the drivers. Only core code should use
> them.

get/put symbols are not exported so I think we should be safe
from driver misuse at this point. If we ever export them we 
should add a 

  WARN_ON(!(devlink->lock_flags & DEVLINK_LOCK_USE_REF));

> Can you please add it as a comment above these functions?

Will do if the RFC sinks.

> At least for now, till we discuss your RFC.

Re: [PATCH net-next v2 3/4] devlink: expose get/put functions

[Leon Romanovsky]

On Mon, Nov 01, 2021 at 06:44:40AM -0700, Jakub Kicinski wrote:
> On Sun, 31 Oct 2021 08:29:20 +0200 Leon Romanovsky wrote:
> > I really like this series, but your latest netdevsim RFC made me worry.
> > 
> > It is important to make sure that these devlink_put() and devlink_get()
> > calls will be out-of-reach from the drivers. Only core code should use
> > them.
> 
> get/put symbols are not exported so I think we should be safe
> from driver misuse at this point. If we ever export them we 
> should add a 
> 
>   WARN_ON(!(devlink->lock_flags & DEVLINK_LOCK_USE_REF));

Right, for now we are safe.

> 
> > Can you please add it as a comment above these functions?
> 
> Will do if the RFC sinks.

I have a solution at hand that doesn't require exporting get/put interfaces.

> 
> > At least for now, till we discuss your RFC.