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

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

[Jakub Kicinski]

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@syzkaller.appspotmail.com
Fixes: 9053637e0da7 ("devlink: remove the registration guarantee of references")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
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);
 }
 
 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);
 	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);
 };
-- 
2.38.1

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

[Jiri Pirko]

Wed, Jan 11, 2023 at 05:29:08AM CET, kuba@kernel.org 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@syzkaller.appspotmail.com
>Fixes: 9053637e0da7 ("devlink: remove the registration guarantee of references")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>


>---
>Jiri, this will likely conflict with your series, sorry :(

Yeah, I will rebase and send v5 after this is applied, no worries.

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

[Jiri Pirko]

Wed, Jan 11, 2023 at 05:29:08AM CET, kuba@kernel.org 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@syzkaller.appspotmail.com
>Fixes: 9053637e0da7 ("devlink: remove the registration guarantee of references")
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reported-by: syzbot+9f0dd863b87113935acf@syzkaller.appspotmail.com

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

[Jacob Keller]

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@syzkaller.appspotmail.com
> Fixes: 9053637e0da7 ("devlink: remove the registration guarantee of references")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Makes sense to me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.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);
>  };

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 10 Jan 2023 20:29:08 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next] devlink: keep the instance mutex alive until references are gone
    https://git.kernel.org/netdev/net-next/c/93e71edfd90c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html