netdev - Re: [PATCH net-next] devlink: keep the instance mutex alive until references are gone

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <09bdd5c7-b452-5924-e9f0-b32bad4d52e3@intel.com>
Date:   Wed, 11 Jan 2023 13:20:11 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, <jiri@...nulli.us>,
        <syzbot+d94d214ea473e218fc89@...kaller.appspotmail.com>
Subject: Re: [PATCH net-next] devlink: keep the instance mutex alive until
 references are gone



On 1/10/2023 8:29 PM, Jakub Kicinski wrote:
> The reference needs to keep the instance memory around, but also
> the instance lock must remain valid. Users will take the lock,
> check registration status and release the lock. mutex_destroy()
> etc. belong in the same place as the freeing of the memory.
> 
> Unfortunately lockdep_unregister_key() sleeps so we need
> to switch the an rcu_work.
> 
> Note that the problem is a bit hard to repro, because
> devlink_pernet_pre_exit() iterates over registered instances.
> AFAIU the instances must get devlink_free()d concurrently with
> the namespace getting deleted for the problem to occur.
> 
> Reported-by: syzbot+d94d214ea473e218fc89@...kaller.appspotmail.com
> Fixes: 9053637e0da7 ("devlink: remove the registration guarantee of references")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Makes sense to me.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

> ---
> Jiri, this will likely conflict with your series, sorry :(
> ---
>  net/devlink/core.c          | 16 +++++++++++++---
>  net/devlink/devl_internal.h |  3 ++-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index a31a317626d7..60beca2df7cc 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -83,10 +83,21 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>  	return NULL;
>  }
>  
> +static void devlink_release(struct work_struct *work)
> +{
> +	struct devlink *devlink;
> +
> +	devlink = container_of(to_rcu_work(work), struct devlink, rwork);
> +
> +	mutex_destroy(&devlink->lock);
> +	lockdep_unregister_key(&devlink->lock_key);
> +	kfree(devlink);
> +}
> +
>  void devlink_put(struct devlink *devlink)
>  {
>  	if (refcount_dec_and_test(&devlink->refcount))
> -		kfree_rcu(devlink, rcu);
> +		queue_rcu_work(system_wq, &devlink->rwork);
>  }
>  

You can't directly call devlink_release because callers of devlink_put
shouldn't sleep. Ok so instead we queue RCU work to do it later. Makes
sense. I was thinking if we'd used kref instead of raw refcount we could
just kref_put, except that just directly calls the release function and
we'd have to queue_rcu_work anyways.

Ok.

>  struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> @@ -231,6 +242,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	INIT_LIST_HEAD(&devlink->trap_list);
>  	INIT_LIST_HEAD(&devlink->trap_group_list);
>  	INIT_LIST_HEAD(&devlink->trap_policer_list);
> +	INIT_RCU_WORK(&devlink->rwork, devlink_release);
>  	lockdep_register_key(&devlink->lock_key);
>  	mutex_init(&devlink->lock);
>  	lockdep_set_class(&devlink->lock, &devlink->lock_key);
> @@ -259,8 +271,6 @@ void devlink_free(struct devlink *devlink)
>  
>  	mutex_destroy(&devlink->linecards_lock);
>  	mutex_destroy(&devlink->reporters_lock);
> -	mutex_destroy(&devlink->lock);
> -	lockdep_unregister_key(&devlink->lock_key);

It seems like we probably would want to move linecards_lock and
reporters_lock too, except we know that these will be removed soon
anyways. Ok.

>  	WARN_ON(!list_empty(&devlink->trap_policer_list));
>  	WARN_ON(!list_empty(&devlink->trap_group_list));
>  	WARN_ON(!list_empty(&devlink->trap_list));
> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> index 5d2bbe295659..e724e4c2a4ff 100644
> --- a/net/devlink/devl_internal.h
> +++ b/net/devlink/devl_internal.h
> @@ -7,6 +7,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/notifier.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>  #include <linux/xarray.h>
>  #include <net/devlink.h>
>  #include <net/net_namespace.h>
> @@ -51,7 +52,7 @@ struct devlink {
>  	struct lock_class_key lock_key;
>  	u8 reload_failed:1;
>  	refcount_t refcount;
> -	struct rcu_head rcu;
> +	struct rcu_work rwork;
>  	struct notifier_block netdevice_nb;
>  	char priv[] __aligned(NETDEV_ALIGN);
>  };