lib: alloc_tag_module_unload must wait for pending kfree_rcu calls

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 1 line

Quoted 3 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

For 6.10+, it implies that the mentioned commit should be backported also.

--
Uladzislau Rezki

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

Quoted 106 lines · Suren Baghdasaryan

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 5 lines

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

For kernels < 6.12, unfortunately not :) If you have a possibility to switch to reclaim over call_rcu() API, then you can go with rcu_barrier() which waits for all callbacks to be completed.

Or backport kvfree_rcu_barrier(). It __should__ be easy since there were not many changes into kvfree_rcu() between 6.10 and 6.12-rcX.

--
Uladzislau Rezki

Quoted 108 lines · Uladzislau Rezki

Quoted 106 lines · Suren Baghdasaryan

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

For kernels < 6.12, unfortunately not :) If you have a possibility to switch to reclaim over call_rcu() API, then you can go with rcu_barrier() which waits for all callbacks to be completed.

We do not control the call-site inside the module, so this would not be possible.

Quoted 3 lines

Or backport kvfree_rcu_barrier(). It __should__ be easy since there were not many changes into kvfree_rcu() between 6.10 and 6.12-rcX.

That sounds better. I'll take a stab at it. Thanks!

Quoted 118 lines · Suren Baghdasaryan

Quoted 116 lines · Uladzislau Rezki

Quoted 106 lines · Suren Baghdasaryan

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 5 lines

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

For kernels < 6.12, unfortunately not :) If you have a possibility to switch to reclaim over call_rcu() API, then you can go with rcu_barrier() which waits for all callbacks to be completed.

Quoted 8 lines

We do not control the call-site inside the module, so this would not be possible.

Quoted 3 lines

Or backport kvfree_rcu_barrier(). It __should__ be easy since there were not many changes into kvfree_rcu() between 6.10 and 6.12-rcX.

That sounds better. I'll take a stab at it. Thanks!

Quoted 1 line

You are welcome!

--
Uladzislau Rezki

Quoted 120 lines · Uladzislau Rezki

Quoted 118 lines · Suren Baghdasaryan

Quoted 116 lines · Uladzislau Rezki

Quoted 106 lines · Suren Baghdasaryan

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 5 lines

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

For kernels < 6.12, unfortunately not :) If you have a possibility to switch to reclaim over call_rcu() API, then you can go with rcu_barrier() which waits for all callbacks to be completed.

Quoted 8 lines

Quoted 8 lines

We do not control the call-site inside the module, so this would not be possible.

Quoted 3 lines

Or backport kvfree_rcu_barrier(). It __should__ be easy since there were not many changes into kvfree_rcu() between 6.10 and 6.12-rcX.

That sounds better. I'll take a stab at it. Thanks!

I prepared backports for stable 6.10.y and 6.11.y branches which contain this patch along with the prerequisite "rcu/kvfree: Add kvfree_rcu_barrier() API" patch. I think that's the only prerequisite required. However, since this patch is not yet in Linus' tree, I'll wait for it to get there before sending backports to stable (per Greg KH's recommendation). Please let me know if you think it's more critical and should be posted earlier.

Quoted 124 lines · Uladzislau Rezki

Quoted 122 lines · Suren Baghdasaryan

Quoted 108 lines · Uladzislau Rezki

Quoted 106 lines · Suren Baghdasaryan

Quoted 74 lines · Uladzislau Rezki

Quoted 72 lines · Suren Baghdasaryan

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.
Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 4 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

Quoted 8 lines

Quoted 5 lines

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

It is because, synchronize_rcu() is used for a bit different things, i.e. it is about a GP completion. Offloading objects can span several GPs.

Ah, thanks! Now that I looked into the patch that recently added kvfree_rcu_barrier() I understand that a bit better.

Quoted 19 lines

Quoted 7 lines

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+

The kvfree_rcu_barrier() has been added into v6.12:

<snip>

urezki@pc638:~/data/raid0/coding/linux.git$ git tag --contains 2b55d6a42d14c8675e38d6d9adca3014fdf01951
next-20240912
next-20240919
next-20240920
next-20241002
v6.12-rc1
urezki@pc638:~/data/raid0/coding/linux.git$
<snip>

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

For 6.10+, it implies that the mentioned commit should be backported also.

I see. I guess for pre-6.12 we would use rcu_barrier() instead of kvfree_rcu_barrier()?

For kernels < 6.12, unfortunately not :) If you have a possibility to switch to reclaim over call_rcu() API, then you can go with rcu_barrier() which waits for all callbacks to be completed.

We do not control the call-site inside the module, so this would not be possible.

Quoted 1 line

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Or backport kvfree_rcu_barrier(). It __should__ be easy since there were not many changes into kvfree_rcu() between 6.10 and 6.12-rcX.

That sounds better. I'll take a stab at it. Thanks!

I prepared backports for stable 6.10.y and 6.11.y branches which

contain this patch along with the prerequisite "rcu/kvfree: Add kvfree_rcu_barrier() API" patch. I think that's the only prerequisite required. However, since this patch is not yet in Linus' tree, I'll wait for it to get there before sending backports to stable (per Greg KH's recommendation). Please let me know if you think it's more critical and should be posted earlier.
Thanks,
Suren.

Quoted 6 lines

Quoted 1 line

You are welcome!

--
Uladzislau Rezki

Quoted 55 lines · Andrew Morton

Quoted 28 lines · On Mon, 7 Oct 2024 22:52:24 +0200 Florian Westphal <[email protected]> wrote:

Ben Greear reports following splat:

 ------------[ cut here ]------------
 net/netfilter/nf_nat_core.c:1114 module nf_nat func:nf_nat_register_fn has 256 allocated at module unload
 WARNING: CPU: 1 PID: 10421 at lib/alloc_tag.c:168 alloc_tag_module_unload+0x22b/0x3f0
 Modules linked in: nf_nat(-) btrfs ufs qnx4 hfsplus hfs minix vfat msdos fat
...
 Hardware name: Default string Default string/SKYBAY, BIOS 5.12 08/04/2020
 RIP: 0010:alloc_tag_module_unload+0x22b/0x3f0
  codetag_unload_module+0x19b/0x2a0
  ? codetag_load_module+0x80/0x80

nf_nat module exit calls kfree_rcu on those addresses, but the free operation is likely still pending by the time alloc_tag checks for leaks.

Wait for outstanding kfree_rcu operations to complete before checking resolves this warning.

Reproducer:
unshare -n iptables-nft -t nat -A PREROUTING -p tcp grep nf_nat /proc/allocinfo # will list 4 allocations rmmod nft_chain_nat
rmmod nf_nat # will WARN.

...

lib/codetag.c bool codetag_unload_module(struct module *mod) · line 235

Quoted 8 lines

lib/codetag.c +2 -0

@@ -228,6 +228,8 @@ bool codetag_unload_module(struct module *mod)
      if (!mod)
              return true;
 
+     kvfree_rcu_barrier();
+
      mutex_lock(&codetag_lock);
      list_for_each_entry(cttype, &codetag_types, link) {
              struct codetag_module *found = NULL;

It's always hard to determine why a thing like this is present, so a comment is helpful:

lib/codetag.c renamed from lib/codetag.c~lib-alloc_tag_module_unload-must-wait-for-pending-kfree_rcu-calls-fix +1 -0

@@ -228,6 +228,7 @@ bool codetag_unload_module(struct module
        if (!mod)
                return true;
 
+       /* await any module's kfree_rcu() operations to complete */
        kvfree_rcu_barrier();
 
        mutex_lock(&codetag_lock);

_

Quoted 4 lines

But I do wonder whether this is in the correct place.

Waiting for a module's ->exit() function's kfree_rcu()s to complete should properly be done by the core module handling code?

I don't think core module code cares about kfree_rcu()s being complete before the module is unloaded.

Allocation tagging OTOH cares because it is about to destroy tags which will be accessed when kfree() actually happens, therefore a strict ordering is important.

Quoted 1 line

Quoted 3 lines

free_module() does a full-on synchronize_rcu() prior to freeing the module memory itself and I think codetag_unload_module() could be called after that?

I think we could move codetag_unload_module() after synchronize_rcu() inside free_module() but according to the reply in
https://lore.kernel.org/all/[email protected]/
synchronize_rcu() does not help. I'm not quite sure why...

Note that once I'm done upstreaming
https://lore.kernel.org/all/[email protected]/,
this change will not be needed and I'm planning to remove this call, however this change is useful for backporting. It should be sent to [email protected] # v6.10+