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 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