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

[PATCH net-next 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.

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 |  16 ++++-
 net/core/dev_ioctl.c  |   2 -
 net/core/devlink.c    |  53 ++++-----------
 net/ethtool/ioctl.c   | 148 ++++++++++++++++++++++++++++++------------
 4 files changed, 134 insertions(+), 85 deletions(-)

-- 
2.31.1

[PATCH net-next 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 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 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 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 |  4 ++--
 net/core/devlink.c    | 45 +++++++------------------------------------
 net/ethtool/ioctl.c   | 36 ++++++++++++++++++++++++++++++----
 3 files changed, 41 insertions(+), 44 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 991ce48f77ca..3d6ae9b546b4 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,
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 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4649 bytes --]

Hi Jakub,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/improve-ethtool-rtnl-vs-devlink-locking/20211030-120714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 28131d896d6d316bc1f6f305d1a9ed6d96c3f2a1
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1ff3b7575e0d00813e134dd4945e5ccab234f2aa
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jakub-Kicinski/improve-ethtool-rtnl-vs-devlink-locking/20211030-120714
        git checkout 1ff3b7575e0d00813e134dd4945e5ccab234f2aa
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/ethtool/ioctl.c: In function 'dev_ethtool':
>> net/ethtool/ioctl.c:3049:63: error: passing argument 1 of 'devlink_compat_flash_update' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3049 |                         rc = devlink_compat_flash_update(state->devlink,
         |                                                          ~~~~~^~~~~~~~~
         |                                                               |
         |                                                               struct devlink *
   In file included from net/ethtool/ioctl.c:28:
   include/net/devlink.h:1757:48: note: expected 'struct net_device *' but argument is of type 'struct devlink *'
    1757 | devlink_compat_flash_update(struct net_device *dev, const char *file_name)
         |                             ~~~~~~~~~~~~~~~~~~~^~~
>> net/ethtool/ioctl.c:3054:61: error: passing argument 1 of 'devlink_compat_running_version' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3054 |                         devlink_compat_running_version(state->devlink,
         |                                                        ~~~~~^~~~~~~~~
         |                                                             |
         |                                                             struct devlink *
   In file included from net/ethtool/ioctl.c:28:
   include/net/devlink.h:1752:51: note: expected 'struct net_device *' but argument is of type 'struct devlink *'
    1752 | devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
         |                                ~~~~~~~~~~~~~~~~~~~^~~
   cc1: some warnings being treated as errors


vim +/devlink_compat_flash_update +3049 net/ethtool/ioctl.c

  3016	
  3017	int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
  3018	{
  3019		struct ethtool_devlink_compat *state;
  3020		u32 ethcmd;
  3021		int rc;
  3022	
  3023		if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
  3024			return -EFAULT;
  3025	
  3026		state = kzalloc(sizeof(*state), GFP_KERNEL);
  3027		if (!state)
  3028			return -ENOMEM;
  3029	
  3030		switch (ethcmd) {
  3031		case ETHTOOL_FLASHDEV:
  3032			if (copy_from_user(&state->efl, useraddr, sizeof(state->efl))) {
  3033				rc = -EFAULT;
  3034				goto exit_free;
  3035			}
  3036			state->efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
  3037			break;
  3038		}
  3039	
  3040		rtnl_lock();
  3041		rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
  3042		rtnl_unlock();
  3043		if (rc)
  3044			goto exit_free;
  3045	
  3046		switch (ethcmd) {
  3047		case ETHTOOL_FLASHDEV:
  3048			if (state->devlink)
> 3049				rc = devlink_compat_flash_update(state->devlink,
  3050								 state->efl.data);
  3051			break;
  3052		case ETHTOOL_GDRVINFO:
  3053			if (state->devlink)
> 3054				devlink_compat_running_version(state->devlink,
  3055							       state->info.fw_version,
  3056							       sizeof(state->info.fw_version));
  3057			if (copy_to_user(useraddr, &state->info, sizeof(state->info))) {
  3058				rc = -EFAULT;
  3059				goto exit_free;
  3060			}
  3061			break;
  3062		}
  3063	
  3064	exit_free:
  3065		if (state->devlink)
  3066			devlink_put(state->devlink);
  3067		kfree(state);
  3068		return rc;
  3069	}
  3070	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19961 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4759 bytes --]

Hi Jakub,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/improve-ethtool-rtnl-vs-devlink-locking/20211030-120714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 28131d896d6d316bc1f6f305d1a9ed6d96c3f2a1
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1ff3b7575e0d00813e134dd4945e5ccab234f2aa
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jakub-Kicinski/improve-ethtool-rtnl-vs-devlink-locking/20211030-120714
        git checkout 1ff3b7575e0d00813e134dd4945e5ccab234f2aa
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/ethtool/ioctl.c: In function 'dev_ethtool':
>> net/ethtool/ioctl.c:3049:63: error: passing argument 1 of 'devlink_compat_flash_update' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3049 |                         rc = devlink_compat_flash_update(state->devlink,
         |                                                          ~~~~~^~~~~~~~~
         |                                                               |
         |                                                               struct devlink *
   In file included from net/ethtool/ioctl.c:28:
   include/net/devlink.h:1757:48: note: expected 'struct net_device *' but argument is of type 'struct devlink *'
    1757 | devlink_compat_flash_update(struct net_device *dev, const char *file_name)
         |                             ~~~~~~~~~~~~~~~~~~~^~~
>> net/ethtool/ioctl.c:3054:61: error: passing argument 1 of 'devlink_compat_running_version' from incompatible pointer type [-Werror=incompatible-pointer-types]
    3054 |                         devlink_compat_running_version(state->devlink,
         |                                                        ~~~~~^~~~~~~~~
         |                                                             |
         |                                                             struct devlink *
   In file included from net/ethtool/ioctl.c:28:
   include/net/devlink.h:1752:51: note: expected 'struct net_device *' but argument is of type 'struct devlink *'
    1752 | devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
         |                                ~~~~~~~~~~~~~~~~~~~^~~
   cc1: some warnings being treated as errors


vim +/devlink_compat_flash_update +3049 net/ethtool/ioctl.c

  3016	
  3017	int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
  3018	{
  3019		struct ethtool_devlink_compat *state;
  3020		u32 ethcmd;
  3021		int rc;
  3022	
  3023		if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
  3024			return -EFAULT;
  3025	
  3026		state = kzalloc(sizeof(*state), GFP_KERNEL);
  3027		if (!state)
  3028			return -ENOMEM;
  3029	
  3030		switch (ethcmd) {
  3031		case ETHTOOL_FLASHDEV:
  3032			if (copy_from_user(&state->efl, useraddr, sizeof(state->efl))) {
  3033				rc = -EFAULT;
  3034				goto exit_free;
  3035			}
  3036			state->efl.data[ETHTOOL_FLASH_MAX_FILENAME - 1] = 0;
  3037			break;
  3038		}
  3039	
  3040		rtnl_lock();
  3041		rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
  3042		rtnl_unlock();
  3043		if (rc)
  3044			goto exit_free;
  3045	
  3046		switch (ethcmd) {
  3047		case ETHTOOL_FLASHDEV:
  3048			if (state->devlink)
> 3049				rc = devlink_compat_flash_update(state->devlink,
  3050								 state->efl.data);
  3051			break;
  3052		case ETHTOOL_GDRVINFO:
  3053			if (state->devlink)
> 3054				devlink_compat_running_version(state->devlink,
  3055							       state->info.fw_version,
  3056							       sizeof(state->info.fw_version));
  3057			if (copy_to_user(useraddr, &state->info, sizeof(state->info))) {
  3058				rc = -EFAULT;
  3059				goto exit_free;
  3060			}
  3061			break;
  3062		}
  3063	
  3064	exit_free:
  3065		if (state->devlink)
  3066			devlink_put(state->devlink);
  3067		kfree(state);
  3068		return rc;
  3069	}
  3070	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 19961 bytes --]