LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] once: Fix panic when module unload
@ 2021-06-22  2:21 Kefeng Wang
  2021-07-16  5:03 ` Kefeng Wang
  2021-08-03  2:11 ` Kefeng Wang
  0 siblings, 2 replies; 5+ messages in thread
From: Kefeng Wang @ 2021-06-22  2:21 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Kefeng Wang, Hannes Frederic Sowa, Daniel Borkmann,
	David S . Miller, Eric Dumazet, Minmin chen

DO_ONCE
DEFINE_STATIC_KEY_TRUE(___once_key);
__do_once_done
  once_disable_jump(once_key);
    INIT_WORK(&w->work, once_deferred);
    struct once_work *w;
    w->key = key;
    schedule_work(&w->work);                     module unload
                                                   //*the key is destroy*
process_one_work
  once_deferred
    BUG_ON(!static_key_enabled(work->key));
       static_key_count((struct static_key *)x)    //*access key, crash*

When module uses DO_ONCE mechanism, it could crash due to the above
concurrency problem, we could reproduce it with link[1].

Fix it by add/put module refcount in the once work process.

[1]
https://lore.kernel.org/netdev/eaa6c371-465e-57eb-6be9-f4b16b9d7cbf@huawei.com/

Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Reported-by: Minmin chen <chenmingmin@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 lib/once.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/once.c b/lib/once.c
index 8b7d6235217e..959f8db41ccf 100644
--- a/lib/once.c
+++ b/lib/once.c
@@ -3,10 +3,12 @@
 #include <linux/spinlock.h>
 #include <linux/once.h>
 #include <linux/random.h>
+#include <linux/module.h>
 
 struct once_work {
 	struct work_struct work;
 	struct static_key_true *key;
+	struct module *module;
 };
 
 static void once_deferred(struct work_struct *w)
@@ -16,11 +18,24 @@ static void once_deferred(struct work_struct *w)
 	work = container_of(w, struct once_work, work);
 	BUG_ON(!static_key_enabled(work->key));
 	static_branch_disable(work->key);
+	module_put(work->module);
 	kfree(work);
 }
 
+static struct module *find_module_by_key(struct static_key_true *key)
+{
+	struct module *mod;
+
+	preempt_disable();
+	mod = __module_address((unsigned long)key);
+	preempt_enable();
+
+	return mod;
+}
+
 static void once_disable_jump(struct static_key_true *key)
 {
+	struct module *mod = find_module_by_key(key);
 	struct once_work *w;
 
 	w = kmalloc(sizeof(*w), GFP_ATOMIC);
@@ -29,6 +44,8 @@ static void once_disable_jump(struct static_key_true *key)
 
 	INIT_WORK(&w->work, once_deferred);
 	w->key = key;
+	w->module = mod;
+	__module_get(mod);
 	schedule_work(&w->work);
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] once: Fix panic when module unload
  2021-06-22  2:21 [PATCH] once: Fix panic when module unload Kefeng Wang
@ 2021-07-16  5:03 ` Kefeng Wang
  2021-08-03  2:11 ` Kefeng Wang
  1 sibling, 0 replies; 5+ messages in thread
From: Kefeng Wang @ 2021-07-16  5:03 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Hannes Frederic Sowa, Daniel Borkmann, David S . Miller,
	Eric Dumazet, Minmin chen

Hi all, kindly ping...

On 2021/6/22 10:21, Kefeng Wang wrote:
> DO_ONCE
> DEFINE_STATIC_KEY_TRUE(___once_key);
> __do_once_done
>    once_disable_jump(once_key);
>      INIT_WORK(&w->work, once_deferred);
>      struct once_work *w;
>      w->key = key;
>      schedule_work(&w->work);                     module unload
>                                                     //*the key is destroy*
> process_one_work
>    once_deferred
>      BUG_ON(!static_key_enabled(work->key));
>         static_key_count((struct static_key *)x)    //*access key, crash*
>
> When module uses DO_ONCE mechanism, it could crash due to the above
> concurrency problem, we could reproduce it with link[1].
>
> Fix it by add/put module refcount in the once work process.
>
> [1]
> https://lore.kernel.org/netdev/eaa6c371-465e-57eb-6be9-f4b16b9d7cbf@huawei.com/
>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Reported-by: Minmin chen <chenmingmin@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   lib/once.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/lib/once.c b/lib/once.c
> index 8b7d6235217e..959f8db41ccf 100644
> --- a/lib/once.c
> +++ b/lib/once.c
> @@ -3,10 +3,12 @@
>   #include <linux/spinlock.h>
>   #include <linux/once.h>
>   #include <linux/random.h>
> +#include <linux/module.h>
>   
>   struct once_work {
>   	struct work_struct work;
>   	struct static_key_true *key;
> +	struct module *module;
>   };
>   
>   static void once_deferred(struct work_struct *w)
> @@ -16,11 +18,24 @@ static void once_deferred(struct work_struct *w)
>   	work = container_of(w, struct once_work, work);
>   	BUG_ON(!static_key_enabled(work->key));
>   	static_branch_disable(work->key);
> +	module_put(work->module);
>   	kfree(work);
>   }
>   
> +static struct module *find_module_by_key(struct static_key_true *key)
> +{
> +	struct module *mod;
> +
> +	preempt_disable();
> +	mod = __module_address((unsigned long)key);
> +	preempt_enable();
> +
> +	return mod;
> +}
> +
>   static void once_disable_jump(struct static_key_true *key)
>   {
> +	struct module *mod = find_module_by_key(key);
>   	struct once_work *w;
>   
>   	w = kmalloc(sizeof(*w), GFP_ATOMIC);
> @@ -29,6 +44,8 @@ static void once_disable_jump(struct static_key_true *key)
>   
>   	INIT_WORK(&w->work, once_deferred);
>   	w->key = key;
> +	w->module = mod;
> +	__module_get(mod);
>   	schedule_work(&w->work);
>   }
>   

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] once: Fix panic when module unload
  2021-06-22  2:21 [PATCH] once: Fix panic when module unload Kefeng Wang
  2021-07-16  5:03 ` Kefeng Wang
@ 2021-08-03  2:11 ` Kefeng Wang
  2021-08-03  9:59   ` Hannes Frederic Sowa
  1 sibling, 1 reply; 5+ messages in thread
From: Kefeng Wang @ 2021-08-03  2:11 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Hannes Frederic Sowa, Daniel Borkmann, David S . Miller,
	Eric Dumazet, Minmin chen, Jakub Kicinski, Greg Kroah-Hartman,
	Andrew Morton

Hi ALL, I don't know who maintain the lib/once.c, add Greg and Andrew too,

Hi David, I check the history, the lib/once.c is from net/core/utils.c 
since

commit 46234253b9363894a254844a6550b4cc5f3edfe8
Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date:   Thu Oct 8 01:20:35 2015 +0200

     net: move net_get_random_once to lib

This bug is found in our product test, we want to make sure that whether 
this solution

is correct or not, so could David or any others help to review this patch.

Many thinks.

On 2021/6/22 10:21, Kefeng Wang wrote:
> DO_ONCE
> DEFINE_STATIC_KEY_TRUE(___once_key);
> __do_once_done
>    once_disable_jump(once_key);
>      INIT_WORK(&w->work, once_deferred);
>      struct once_work *w;
>      w->key = key;
>      schedule_work(&w->work);                     module unload
>                                                     //*the key is destroy*
> process_one_work
>    once_deferred
>      BUG_ON(!static_key_enabled(work->key));
>         static_key_count((struct static_key *)x)    //*access key, crash*
>
> When module uses DO_ONCE mechanism, it could crash due to the above
> concurrency problem, we could reproduce it with link[1].
>
> Fix it by add/put module refcount in the once work process.
>
> [1]
> https://lore.kernel.org/netdev/eaa6c371-465e-57eb-6be9-f4b16b9d7cbf@huawei.com/
>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Reported-by: Minmin chen <chenmingmin@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>   lib/once.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/lib/once.c b/lib/once.c
> index 8b7d6235217e..959f8db41ccf 100644
> --- a/lib/once.c
> +++ b/lib/once.c
> @@ -3,10 +3,12 @@
>   #include <linux/spinlock.h>
>   #include <linux/once.h>
>   #include <linux/random.h>
> +#include <linux/module.h>
>   
>   struct once_work {
>   	struct work_struct work;
>   	struct static_key_true *key;
> +	struct module *module;
>   };
>   
>   static void once_deferred(struct work_struct *w)
> @@ -16,11 +18,24 @@ static void once_deferred(struct work_struct *w)
>   	work = container_of(w, struct once_work, work);
>   	BUG_ON(!static_key_enabled(work->key));
>   	static_branch_disable(work->key);
> +	module_put(work->module);
>   	kfree(work);
>   }
>   
> +static struct module *find_module_by_key(struct static_key_true *key)
> +{
> +	struct module *mod;
> +
> +	preempt_disable();
> +	mod = __module_address((unsigned long)key);
> +	preempt_enable();
> +
> +	return mod;
> +}
> +
>   static void once_disable_jump(struct static_key_true *key)
>   {
> +	struct module *mod = find_module_by_key(key);
>   	struct once_work *w;
>   
>   	w = kmalloc(sizeof(*w), GFP_ATOMIC);
> @@ -29,6 +44,8 @@ static void once_disable_jump(struct static_key_true *key)
>   
>   	INIT_WORK(&w->work, once_deferred);
>   	w->key = key;
> +	w->module = mod;
> +	__module_get(mod);
>   	schedule_work(&w->work);
>   }
>   

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] once: Fix panic when module unload
  2021-08-03  2:11 ` Kefeng Wang
@ 2021-08-03  9:59   ` Hannes Frederic Sowa
  2021-08-04  1:49     ` Kefeng Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Hannes Frederic Sowa @ 2021-08-03  9:59 UTC (permalink / raw)
  To: Kefeng Wang, linux-kernel, netdev
  Cc: Daniel Borkmann, David Miller, Eric Dumazet, Minmin chen,
	Jakub Kicinski, Greg Kroah-Hartman, Andrew Morton

Hello,

On Tue, Aug 3, 2021, at 04:11, Kefeng Wang wrote:
> Hi ALL, I don't know who maintain the lib/once.c, add Greg and Andrew too,
> 
> Hi David, I check the history, the lib/once.c is from net/core/utils.c 
> since
> 
> commit 46234253b9363894a254844a6550b4cc5f3edfe8
> Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date:   Thu Oct 8 01:20:35 2015 +0200
> 
>      net: move net_get_random_once to lib
> 
> This bug is found in our product test, we want to make sure that whether 
> this solution
> 
> is correct or not, so could David or any others help to review this patch.
> 
> Many thinks.

Thanks for the patch.

I see that it got marked as not applicable for the net trees:
<https://patchwork.kernel.org/project/netdevbpf/patch/20210622022138.23048-1-wangkefeng.wang@huawei.com/>

Back then I added this code via the net/ tree thus I think it should get
picked up nonetheless hopefully.

Regarding your patch, I think it mostly looks fine:

It might be worthwhile to increment the reference counter inside the
preempt disabled bracket in find_module_by_key (and thus also rename
that function to make this fact more clear).

The other option would be to use the macro DO_ONCE and always pass in
THIS_MODULE from there, increment its ref counter in once_disable_jump.
This might be more canonical.

Thanks and sorry for the delay,
Hannes

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] once: Fix panic when module unload
  2021-08-03  9:59   ` Hannes Frederic Sowa
@ 2021-08-04  1:49     ` Kefeng Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Kefeng Wang @ 2021-08-04  1:49 UTC (permalink / raw)
  To: Hannes Frederic Sowa, linux-kernel, netdev
  Cc: Daniel Borkmann, David Miller, Eric Dumazet, Minmin chen,
	Jakub Kicinski, Greg Kroah-Hartman, Andrew Morton


On 2021/8/3 17:59, Hannes Frederic Sowa wrote:
> Hello,
>
> On Tue, Aug 3, 2021, at 04:11, Kefeng Wang wrote:
>> Hi ALL, I don't know who maintain the lib/once.c, add Greg and Andrew too,
>>
>> Hi David, I check the history, the lib/once.c is from net/core/utils.c
>> since
>>
>> commit 46234253b9363894a254844a6550b4cc5f3edfe8
>> Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Date:   Thu Oct 8 01:20:35 2015 +0200
>>
>>       net: move net_get_random_once to lib
>>
>> This bug is found in our product test, we want to make sure that whether
>> this solution
>>
>> is correct or not, so could David or any others help to review this patch.
>>
>> Many thinks.
> Thanks for the patch.
>
> I see that it got marked as not applicable for the net trees:
> <https://patchwork.kernel.org/project/netdevbpf/patch/20210622022138.23048-1-wangkefeng.wang@huawei.com/>
>
> Back then I added this code via the net/ tree thus I think it should get
> picked up nonetheless hopefully.
>
> Regarding your patch, I think it mostly looks fine:
>
> It might be worthwhile to increment the reference counter inside the
> preempt disabled bracket in find_module_by_key (and thus also rename
> that function to make this fact more clear).
>
> The other option would be to use the macro DO_ONCE and always pass in
> THIS_MODULE from there, increment its ref counter in once_disable_jump.
> This might be more canonical.

Thanks for your replay.

Yes, that was my first thought, add THIS_MODULE to __do_once_done(),

I will change to this way to fix the issue.


>
> Thanks and sorry for the delay,
> Hannes
> .
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-04  1:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22  2:21 [PATCH] once: Fix panic when module unload Kefeng Wang
2021-07-16  5:03 ` Kefeng Wang
2021-08-03  2:11 ` Kefeng Wang
2021-08-03  9:59   ` Hannes Frederic Sowa
2021-08-04  1:49     ` Kefeng Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).