| 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
| ||
|
Message-ID: <84151471-4404-d944-417f-2982569f44da@intel.com>
Date: Mon, 19 Dec 2022 09:48:54 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <jiri@...nulli.us>,
<leon@...nel.org>
CC: <netdev@...r.kernel.org>
Subject: Re: [RFC net-next 04/10] devlink: always check if the devlink
instance is registered
On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> Always check under the instance lock whether the devlink instance
> is still / already registered.
>
Ok. So now the reference ensures less about whats valid. It guarantees a
lock but doesn't ensure that the devlink remains registered unless you
acquire the lock and check that the devlink is alive under lock now?
> This is a no-op for the most part, as the unregistration path currently
> waits for all references. On the init path, however, we may temporarily
> open up a race with netdev code, if netdevs are registered before the
> devlink instance. This is temporary, the next change fixes it, and this
> commit has been split out for the ease of review.
>
This means you're adding the problem here, but its fixed in next commit..?
> Note that in case of iterating over sub-objects which have their
> own lock (regions and line cards) we assume an implicit dependency
> between those objects existing and devlink unregistration.
>
That seems reasonable.
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> include/net/devlink.h | 1 +
> net/devlink/basic.c | 35 +++++++++++++++++++++++++++++------
> net/devlink/core.c | 25 +++++++++++++++++++++----
> net/devlink/netlink.c | 10 ++++++++--
> 4 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 6a2e4f21779f..36e013d3aa52 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
> void devl_lock(struct devlink *devlink);
> int devl_trylock(struct devlink *devlink);
> void devl_unlock(struct devlink *devlink);
> +bool devl_is_alive(struct devlink *devlink);
> void devl_assert_locked(struct devlink *devlink);
> bool devl_lock_is_held(struct devlink *devlink);
>
> diff --git a/net/devlink/basic.c b/net/devlink/basic.c
> index 5f33d74eef83..6b18e70a39fd 100644
> --- a/net/devlink/basic.c
> +++ b/net/devlink/basic.c
> @@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> int idx = 0;
>
> mutex_lock(&devlink->linecards_lock);
> + if (!devl_is_alive(devlink))
> + goto next_devlink;
> +
> list_for_each_entry(linecard, &devlink->linecard_list, list) {
> if (idx < dump->idx) {
> idx++;
> @@ -2151,6 +2154,7 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> }
> idx++;
> }
> +next_devlink:
> mutex_unlock(&devlink->linecards_lock);
> devlink_put(devlink);
> }
> @@ -7809,6 +7813,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> int idx = 0;
>
> mutex_lock(&devlink->reporters_lock);
> + if (!devl_is_alive(devlink)) {
> + mutex_unlock(&devlink->reporters_lock);
> + devlink_put(devlink);
> + continue;
> + }
> +
> list_for_each_entry(reporter, &devlink->reporter_list,
> list) {
> if (idx < dump->idx) {
> @@ -7830,6 +7840,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> mutex_unlock(&devlink->reporters_lock);
>
> devl_lock(devlink);
> + if (!devl_is_alive(devlink))
> + goto next_devlink;
> +
> xa_for_each(&devlink->ports, port_index, port) {
> mutex_lock(&port->reporters_lock);
> list_for_each_entry(reporter, &port->reporter_list, list) {
> @@ -7853,6 +7866,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> }
> mutex_unlock(&port->reporters_lock);
> }
> +next_devlink:
> devl_unlock(devlink);
> devlink_put(devlink);
> }
> @@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
> return;
>
> devl_lock(devlink);
> - __devlink_compat_running_version(devlink, buf, len);
> + if (devl_is_alive(devlink))
> + __devlink_compat_running_version(devlink, buf, len);
> devl_unlock(devlink);
> }
>
> @@ -12227,20 +12242,28 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
> struct devlink_flash_update_params params = {};
> int ret;
>
> - if (!devlink->ops->flash_update)
> - return -EOPNOTSUPP;
> + devl_lock(devlink);
> + if (!devl_is_alive(devlink)) {
> + ret = -ENODEV;
> + goto out_unlock;
> + }
> +
> + if (!devlink->ops->flash_update) {
> + ret = -EOPNOTSUPP;
> + goto out_unlock;
> + }
>
> ret = request_firmware(¶ms.fw, file_name, devlink->dev);
> if (ret)
> - return ret;
> + goto out_unlock;
>
> - devl_lock(devlink);
> devlink_flash_update_begin_notify(devlink);
> ret = devlink->ops->flash_update(devlink, ¶ms, NULL);
> devlink_flash_update_end_notify(devlink);
> - devl_unlock(devlink);
>
> release_firmware(params.fw);
> +out_unlock:
> + devl_unlock(devlink);
>
> return ret;
> }
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index d3b8336946fd..2abad8247597 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -67,6 +67,21 @@ void devl_unlock(struct devlink *devlink)
> }
> EXPORT_SYMBOL_GPL(devl_unlock);
>
> +bool devl_is_alive(struct devlink *devlink)
> +{
> + return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> +}
> +EXPORT_SYMBOL_GPL(devl_is_alive);
> +
> +/**
> + * devlink_try_get() - try to obtain a reference on a devlink instance
> + * @devlink: instance to reference
> + *
> + * Obtain a reference on a devlink instance. A reference on a devlink instance
> + * only implies that it's safe to take the instance lock. It does not imply
> + * that the instance is registered, use devl_is_alive() after taking
> + * the instance lock to check registration status.
> + */
> struct devlink *__must_check devlink_try_get(struct devlink *devlink)
> {
> if (refcount_inc_not_zero(&devlink->refcount))
> @@ -300,10 +315,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
> devlinks_xa_for_each_registered_get(net, index, devlink) {
> WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
> devl_lock(devlink);
> - err = devlink_reload(devlink, &init_net,
> - DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> - DEVLINK_RELOAD_LIMIT_UNSPEC,
> - &actions_performed, NULL);
> + err = 0;
> + if (devl_is_alive(devlink))
> + err = devlink_reload(devlink, &init_net,
> + DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> + DEVLINK_RELOAD_LIMIT_UNSPEC,
> + &actions_performed, NULL);
> devl_unlock(devlink);
> devlink_put(devlink);
>
> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
> index b38df704be1c..773efaabb6ad 100644
> --- a/net/devlink/netlink.c
> +++ b/net/devlink/netlink.c
> @@ -98,7 +98,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
>
> devlinks_xa_for_each_registered_get(net, index, devlink) {
> devl_lock(devlink);
> - if (strcmp(devlink->dev->bus->name, busname) == 0 &&
> + if (devl_is_alive(devlink) &&
> + strcmp(devlink->dev->bus->name, busname) == 0 &&
> strcmp(dev_name(devlink->dev), devname) == 0)
> return devlink;
> devl_unlock(devlink);
> @@ -210,7 +211,12 @@ int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
>
> devlink_dump_for_each_instance_get(msg, dump, devlink) {
> devl_lock(devlink);
> - err = cmd->dump_one(msg, devlink, cb);
> +
> + if (devl_is_alive(devlink))
> + err = cmd->dump_one(msg, devlink, cb);
> + else
> + err = 0;
> +
> devl_unlock(devlink);
> devlink_put(devlink);
>
Powered by blists - more mailing lists