* [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking
@ 2021-10-30 4:06 Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 1/4] ethtool: push the rtnl_lock into dev_ethtool() Jakub Kicinski
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-30 4:06 UTC (permalink / raw)
To: davem; +Cc: netdev, jiri, leon, mkubecek, andrew, f.fainelli, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/4] ethtool: push the rtnl_lock into dev_ethtool()
2021-10-30 4:06 [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking Jakub Kicinski
@ 2021-10-30 4:06 ` Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 2/4] ethtool: handle info/flash data copying outside rtnl_lock Jakub Kicinski
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-30 4:06 UTC (permalink / raw)
To: davem; +Cc: netdev, jiri, leon, mkubecek, andrew, f.fainelli, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/4] ethtool: handle info/flash data copying outside rtnl_lock
2021-10-30 4:06 [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 1/4] ethtool: push the rtnl_lock into dev_ethtool() Jakub Kicinski
@ 2021-10-30 4:06 ` Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 3/4] devlink: expose get/put functions Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl Jakub Kicinski
3 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-30 4:06 UTC (permalink / raw)
To: davem; +Cc: netdev, jiri, leon, mkubecek, andrew, f.fainelli, 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(ðcmd, 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(ðcmd, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 3/4] devlink: expose get/put functions
2021-10-30 4:06 [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 1/4] ethtool: push the rtnl_lock into dev_ethtool() Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 2/4] ethtool: handle info/flash data copying outside rtnl_lock Jakub Kicinski
@ 2021-10-30 4:06 ` Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl Jakub Kicinski
3 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-30 4:06 UTC (permalink / raw)
To: davem; +Cc: netdev, jiri, leon, mkubecek, andrew, f.fainelli, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl
2021-10-30 4:06 [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking Jakub Kicinski
` (2 preceding siblings ...)
2021-10-30 4:06 ` [PATCH net-next 3/4] devlink: expose get/put functions Jakub Kicinski
@ 2021-10-30 4:06 ` Jakub Kicinski
2021-10-30 8:50 ` kernel test robot
3 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-10-30 4:06 UTC (permalink / raw)
To: davem; +Cc: netdev, jiri, leon, mkubecek, andrew, f.fainelli, 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(¶ms.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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl
2021-10-30 4:06 ` [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl Jakub Kicinski
@ 2021-10-30 8:50 ` kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-10-30 8:50 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: kbuild-all, netdev, jiri, leon, mkubecek, andrew, f.fainelli,
Jakub Kicinski
[-- 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(ðcmd, 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl
@ 2021-10-30 8:50 ` kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-10-30 8:50 UTC (permalink / raw)
To: kbuild-all
[-- 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(ðcmd, 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-30 8:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-30 4:06 [PATCH net-next 0/4] improve ethtool/rtnl vs devlink locking Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 1/4] ethtool: push the rtnl_lock into dev_ethtool() Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 2/4] ethtool: handle info/flash data copying outside rtnl_lock Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 3/4] devlink: expose get/put functions Jakub Kicinski
2021-10-30 4:06 ` [PATCH net-next 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl Jakub Kicinski
2021-10-30 8:50 ` kernel test robot
2021-10-30 8:50 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.