| 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: <ac6f8ab5-3838-4686-fc20-b98b196f82c8@intel.com>
Date: Mon, 19 Dec 2022 09:56:26 -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 05/10] devlink: remove the registration guarantee
of references
On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> The objective of exposing the devlink instance locks to
> drivers was to let them use these locks to prevent user space
> from accessing the device before it's fully initialized.
> This is difficult because devlink_unregister() waits for all
> references to be released, meaning that devlink_unregister()
> can't itself be called under the instance lock.
>
Sure.
> To avoid this issue devlink_register() was moved after subobject
> registration a while ago. Unfortunately the netdev paths get
> a hold of the devlink instances _before_ they are registered.
> Ideally netdev should wait for devlink init to finish (synchronizing
> on the instance lock). This can't work because we don't know if the
> instance will _ever_ be registered (in case of failures it may not).
> The other option of returning an error until devlink_register()
> is called is unappealing (user space would get a notification
> netdev exist but would have to wait arbitrary amount of time
> before accessing some of its attributes).
>
Nice summary of the problems and options that we have tried already.
I think its also important as this can allow sub objects to be
registered after the devlink instance?
> Weaken the guarantees of the devlink references.
>
> Holding a reference will now only guarantee that the memory
> of the object is around. Another way of looking at it is that
> the reference now protects the object not its "registered" status.
> Use devlink instance lock to synchronize unregistration.
>
Right, this makes sense.
> This implies that releasing of the "main" reference of the devlink
> instance moves from devlink_unregister() to devlink_free().
>
This makes sense and I think aligns more with how most references work
in practice. Good.
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
Code change seems straight forward enough. I had a minor question, but:
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
> include/net/devlink.h | 2 ++
> net/devlink/core.c | 64 ++++++++++++++++---------------------
> net/devlink/devl_internal.h | 2 --
> 3 files changed, 30 insertions(+), 38 deletions(-)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 36e013d3aa52..cc910612b3f4 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1648,6 +1648,8 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
> return devlink_alloc_ns(ops, priv_size, &init_net, dev);
> }
> void devlink_set_features(struct devlink *devlink, u64 features);
> +int devl_register(struct devlink *devlink);
> +void devl_unregister(struct devlink *devlink);
> void devlink_register(struct devlink *devlink);
> void devlink_unregister(struct devlink *devlink);
> void devlink_free(struct devlink *devlink);
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index 2abad8247597..413b92534ad6 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -89,21 +89,10 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
> return NULL;
> }
>
> -static void __devlink_put_rcu(struct rcu_head *head)
> -{
> - struct devlink *devlink = container_of(head, struct devlink, rcu);
> -
> - complete(&devlink->comp);
> -}
> -
> void devlink_put(struct devlink *devlink)
> {
> if (refcount_dec_and_test(&devlink->refcount))
> - /* Make sure unregister operation that may await the completion
> - * is unblocked only after all users are after the end of
> - * RCU grace period.
> - */
> - call_rcu(&devlink->rcu, __devlink_put_rcu);
> + kfree_rcu(devlink, rcu);
> }
>
> struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> @@ -116,13 +105,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> if (!devlink)
> goto unlock;
>
> - /* In case devlink_unregister() was already called and "unregistering"
> - * mark was set, do not allow to get a devlink reference here.
> - * This prevents live-lock of devlink_unregister() wait for completion.
> - */
> - if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
> - goto next;
> -
> if (!devlink_try_get(devlink))
> goto next;
> if (!net_eq(devlink_net(devlink), net)) {
> @@ -158,37 +140,48 @@ void devlink_set_features(struct devlink *devlink, u64 features)
> EXPORT_SYMBOL_GPL(devlink_set_features);
>
> /**
> - * devlink_register - Register devlink instance
> - *
> - * @devlink: devlink
> + * devl_register - Register devlink instance
> + * @devlink: devlink
> */
> -void devlink_register(struct devlink *devlink)
> +int devl_register(struct devlink *devlink)
> {
> ASSERT_DEVLINK_NOT_REGISTERED(devlink);
> - /* Make sure that we are in .probe() routine */
> + devl_assert_locked(devlink);
>
> xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> devlink_notify_register(devlink);
> +
> + return 0;
Any particular reason to change this to int when it doesn't have a
failure case yet? Future patches I assume? You don't check the
devl_register return value.
> +}
> +EXPORT_SYMBOL_GPL(devl_register);
> +
> +void devlink_register(struct devlink *devlink)
> +{
> + devl_lock(devlink);
> + devl_register(devlink);
> + devl_unlock(devlink);
> }
> EXPORT_SYMBOL_GPL(devlink_register);
>
> /**
> - * devlink_unregister - Unregister devlink instance
> - *
> - * @devlink: devlink
> + * devl_unregister - Unregister devlink instance
> + * @devlink: devlink
> */
> -void devlink_unregister(struct devlink *devlink)
> +void devl_unregister(struct devlink *devlink)
> {
> ASSERT_DEVLINK_REGISTERED(devlink);
> - /* Make sure that we are in .remove() routine */
> -
> - xa_set_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
> - devlink_put(devlink);
> - wait_for_completion(&devlink->comp);
> + devl_assert_locked(devlink);
>
> devlink_notify_unregister(devlink);
> xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> - xa_clear_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
> +}
> +EXPORT_SYMBOL_GPL(devl_unregister);
> +
> +void devlink_unregister(struct devlink *devlink)
> +{
> + devl_lock(devlink);
> + devl_unregister(devlink);
> + devl_unlock(devlink);
> }
> EXPORT_SYMBOL_GPL(devlink_unregister);
>
> @@ -252,7 +245,6 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
> mutex_init(&devlink->reporters_lock);
> mutex_init(&devlink->linecards_lock);
> refcount_set(&devlink->refcount, 1);
> - init_completion(&devlink->comp);
>
> return devlink;
>
> @@ -298,7 +290,7 @@ void devlink_free(struct devlink *devlink)
>
> xa_erase(&devlinks, devlink->index);
>
> - kfree(devlink);
> + devlink_put(devlink);
> }
> EXPORT_SYMBOL_GPL(devlink_free);
>
> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> index c3977c69552a..7e77eebde3b9 100644
> --- a/net/devlink/devl_internal.h
> +++ b/net/devlink/devl_internal.h
> @@ -12,7 +12,6 @@
> #include <net/net_namespace.h>
>
> #define DEVLINK_REGISTERED XA_MARK_1
> -#define DEVLINK_UNREGISTERING XA_MARK_2
>
> #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
> (__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
> @@ -52,7 +51,6 @@ struct devlink {
> struct lock_class_key lock_key;
> u8 reload_failed:1;
> refcount_t refcount;
> - struct completion comp;
> struct rcu_head rcu;
> struct notifier_block netdevice_nb;
> char priv[] __aligned(NETDEV_ALIGN);
Powered by blists - more mailing lists