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