From f49deaa64af10276ef0c9a09558152f990b5f3b1 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Sat, 30 Oct 2021 10:18:48 -0700 Subject: [PATCH 1/4] ethtool: push the rtnl_lock into dev_ethtool() 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 Reviewed-by: Leon Romanovsky Signed-off-by: David S. Miller --- 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 { From 095cfcfe13e5a6599cf0a41fe1e8bbfa76cd1c9d Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Sat, 30 Oct 2021 10:18:49 -0700 Subject: [PATCH 2/4] ethtool: handle info/flash data copying outside rtnl_lock 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 Reviewed-by: Leon Romanovsky Signed-off-by: David S. Miller --- net/ethtool/ioctl.c | 114 +++++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 43 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 #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); + rsp->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)); - - 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; - rtnl_lock(); - rc = __dev_ethtool(net, ifr, useraddr); - rtnl_unlock(); + 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, 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; } From 46db1b77cd4f082c4e908c8f8086a2f7aae28b62 Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Sat, 30 Oct 2021 10:18:50 -0700 Subject: [PATCH 3/4] devlink: expose get/put functions 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 Reviewed-by: Leon Romanovsky Signed-off-by: David S. Miller --- 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, From 1af0a0948e28d83bcfa9d48cd0f992f616c5d62e Mon Sep 17 00:00:00 2001 From: Jakub Kicinski Date: Sat, 30 Oct 2021 10:18:51 -0700 Subject: [PATCH 4/4] ethtool: don't drop the rtnl_lock half way thru the ioctl 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 Reviewed-by: Leon Romanovsky Signed-off-by: David S. Miller --- 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(¶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; }