| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 5 Aug 2022 16:21:16 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Jiri Pirko <jiri@...nulli.us>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"idosch@...dia.com" <idosch@...dia.com>,
"petrm@...dia.com" <petrm@...dia.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"mlxsw@...dia.com" <mlxsw@...dia.com>,
"saeedm@...dia.com" <saeedm@...dia.com>,
"tariqt@...dia.com" <tariqt@...dia.com>,
"leon@...nel.org" <leon@...nel.org>,
"moshe@...dia.com" <moshe@...dia.com>
Subject: RE: [patch net-next 2/4] net: devlink: convert reload command to take
implicit devlink->lock
> -----Original Message-----
> From: Jiri Pirko <jiri@...nulli.us>
> Sent: Friday, July 29, 2022 12:11 AM
> To: netdev@...r.kernel.org
> Cc: davem@...emloft.net; kuba@...nel.org; idosch@...dia.com;
> petrm@...dia.com; pabeni@...hat.com; edumazet@...gle.com;
> mlxsw@...dia.com; saeedm@...dia.com; tariqt@...dia.com; leon@...nel.org;
> moshe@...dia.com
> Subject: [patch net-next 2/4] net: devlink: convert reload command to take
> implicit devlink->lock
>
> From: Jiri Pirko <jiri@...dia.com>
>
> Convert reload command to behave the same way as the rest of the
> commands and let if be called with devlink->lock held. Remove the
> temporary devl_lock taking from drivers. As the DEVLINK_NL_FLAG_NO_LOCK
> flag is no longer used, remove it alongside.
>
> Signed-off-by: Jiri Pirko <jiri@...dia.com>
Wasn't reload avoiding the lock done so that drivers could perform other devlink operations during reload? Or is that no longer a problem with the recent refactors done to move devlink initialization and such earlier so that its now safe?
Thanks,
Jake
> ---
> drivers/net/ethernet/mellanox/mlx4/main.c | 4 ----
> .../net/ethernet/mellanox/mlx5/core/devlink.c | 4 ----
> drivers/net/ethernet/mellanox/mlxsw/core.c | 4 ----
> drivers/net/netdevsim/dev.c | 6 ------
> net/core/devlink.c | 18 +++++-------------
> 5 files changed, 5 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c
> b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 2c764d1d897d..78c5f40382c9 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -3958,11 +3958,9 @@ static int mlx4_devlink_reload_down(struct devlink
> *devlink, bool netns_change,
> NL_SET_ERR_MSG_MOD(extack, "Namespace change is not
> supported");
> return -EOPNOTSUPP;
> }
> - devl_lock(devlink);
> if (persist->num_vfs)
> mlx4_warn(persist->dev, "Reload performed on PF, will cause
> reset on operating Virtual Functions\n");
> mlx4_restart_one_down(persist->pdev);
> - devl_unlock(devlink);
> return 0;
> }
>
> @@ -3975,10 +3973,8 @@ static int mlx4_devlink_reload_up(struct devlink
> *devlink, enum devlink_reload_a
> struct mlx4_dev_persistent *persist = dev->persist;
> int err;
>
> - devl_lock(devlink);
> *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT);
> err = mlx4_restart_one_up(persist->pdev, true, devlink);
> - devl_unlock(devlink);
> if (err)
> mlx4_err(persist->dev, "mlx4_restart_one_up failed, ret=%d\n",
> err);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> index 1c05a7091698..66c6a7017695 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> @@ -164,7 +164,6 @@ static int mlx5_devlink_reload_down(struct devlink
> *devlink, bool netns_change,
> NL_SET_ERR_MSG_MOD(extack, "reload while VFs are present is
> unfavorable");
> }
>
> - devl_lock(devlink);
> switch (action) {
> case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
> mlx5_unload_one_devl_locked(dev);
> @@ -181,7 +180,6 @@ static int mlx5_devlink_reload_down(struct devlink
> *devlink, bool netns_change,
> ret = -EOPNOTSUPP;
> }
>
> - devl_unlock(devlink);
> return ret;
> }
>
> @@ -192,7 +190,6 @@ static int mlx5_devlink_reload_up(struct devlink *devlink,
> enum devlink_reload_a
> struct mlx5_core_dev *dev = devlink_priv(devlink);
> int ret = 0;
>
> - devl_lock(devlink);
> *actions_performed = BIT(action);
> switch (action) {
> case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
> @@ -211,7 +208,6 @@ static int mlx5_devlink_reload_up(struct devlink *devlink,
> enum devlink_reload_a
> ret = -EOPNOTSUPP;
> }
>
> - devl_unlock(devlink);
> return ret;
> }
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c
> b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index a48f893cf7b0..e12918b6baa1 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -1499,9 +1499,7 @@ mlxsw_devlink_core_bus_device_reload_down(struct
> devlink *devlink,
> if (!(mlxsw_core->bus->features & MLXSW_BUS_F_RESET))
> return -EOPNOTSUPP;
>
> - devl_lock(devlink);
> mlxsw_core_bus_device_unregister(mlxsw_core, true);
> - devl_unlock(devlink);
> return 0;
> }
>
> @@ -1515,12 +1513,10 @@ mlxsw_devlink_core_bus_device_reload_up(struct
> devlink *devlink, enum devlink_re
>
> *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
> BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE);
> - devl_lock(devlink);
> err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
> mlxsw_core->bus,
> mlxsw_core->bus_priv, true,
> devlink, extack);
> - devl_unlock(devlink);
> return err;
> }
>
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index 5802e80e8fe1..e88f783c297e 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -948,18 +948,15 @@ static int nsim_dev_reload_down(struct devlink
> *devlink, bool netns_change,
> {
> struct nsim_dev *nsim_dev = devlink_priv(devlink);
>
> - devl_lock(devlink);
> if (nsim_dev->dont_allow_reload) {
> /* For testing purposes, user set debugfs dont_allow_reload
> * value to true. So forbid it.
> */
> NL_SET_ERR_MSG_MOD(extack, "User forbid the reload for
> testing purposes");
> - devl_unlock(devlink);
> return -EOPNOTSUPP;
> }
>
> nsim_dev_reload_destroy(nsim_dev);
> - devl_unlock(devlink);
> return 0;
> }
>
> @@ -970,19 +967,16 @@ static int nsim_dev_reload_up(struct devlink *devlink,
> enum devlink_reload_actio
> struct nsim_dev *nsim_dev = devlink_priv(devlink);
> int ret;
>
> - devl_lock(devlink);
> if (nsim_dev->fail_reload) {
> /* For testing purposes, user set debugfs fail_reload
> * value to true. Fail right away.
> */
> NL_SET_ERR_MSG_MOD(extack, "User setup the reload to fail for
> testing purposes");
> - devl_unlock(devlink);
> return -EINVAL;
> }
>
> *actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT);
> ret = nsim_dev_reload_create(nsim_dev, extack);
> - devl_unlock(devlink);
> return ret;
> }
>
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 6b20196ada1a..57865b231364 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -768,12 +768,6 @@ devlink_region_snapshot_get_by_id(struct
> devlink_region *region, u32 id)
> #define DEVLINK_NL_FLAG_NEED_RATE_NODE BIT(3)
> #define DEVLINK_NL_FLAG_NEED_LINECARD BIT(4)
>
> -/* The per devlink instance lock is taken by default in the pre-doit
> - * operation, yet several commands do not require this. The global
> - * devlink lock is taken and protects from disruption by user-calls.
> - */
> -#define DEVLINK_NL_FLAG_NO_LOCK BIT(5)
> -
> static int devlink_nl_pre_doit(const struct genl_ops *ops,
> struct sk_buff *skb, struct genl_info *info)
> {
> @@ -788,8 +782,7 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> mutex_unlock(&devlink_mutex);
> return PTR_ERR(devlink);
> }
> - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> - devl_lock(devlink);
> + devl_lock(devlink);
> info->user_ptr[0] = devlink;
> if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_PORT) {
> devlink_port = devlink_port_get_from_info(devlink, info);
> @@ -831,8 +824,7 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> return 0;
>
> unlock:
> - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> - devl_unlock(devlink);
> + devl_unlock(devlink);
> devlink_put(devlink);
> mutex_unlock(&devlink_mutex);
> return err;
> @@ -849,8 +841,7 @@ static void devlink_nl_post_doit(const struct genl_ops
> *ops,
> linecard = info->user_ptr[1];
> devlink_linecard_put(linecard);
> }
> - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> - devl_unlock(devlink);
> + devl_unlock(devlink);
> devlink_put(devlink);
> mutex_unlock(&devlink_mutex);
> }
> @@ -9414,7 +9405,6 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> .validate = GENL_DONT_VALIDATE_STRICT |
> GENL_DONT_VALIDATE_DUMP,
> .doit = devlink_nl_cmd_reload,
> .flags = GENL_ADMIN_PERM,
> - .internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
> },
> {
> .cmd = DEVLINK_CMD_PARAM_GET,
> @@ -12507,10 +12497,12 @@ static void __net_exit
> devlink_pernet_pre_exit(struct net *net)
> mutex_lock(&devlink_mutex);
> devlinks_xa_for_each_registered_get(net, index, devlink) {
> WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
> + mutex_lock(&devlink->lock);
> err = devlink_reload(devlink, &init_net,
> DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> DEVLINK_RELOAD_LIMIT_UNSPEC,
> &actions_performed, NULL);
> + mutex_unlock(&devlink->lock);
> if (err && err != -EOPNOTSUPP)
> pr_warn("Failed to reload devlink instance into
> init_net\n");
> devlink_put(devlink);
> --
> 2.35.3
Powered by blists - more mailing lists