diff mbox series

[net] netfilter: nf_tables: prefer kfree_rcu(ptr, rcu) variant

Message ID 20220222181331.811085-1-eric.dumazet@gmail.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [net] netfilter: nf_tables: prefer kfree_rcu(ptr, rcu) variant | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/cc_maintainers warning 4 maintainers not CCed: kadlec@netfilter.org davem@davemloft.net kuba@kernel.org coreteam@netfilter.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet Feb. 22, 2022, 6:13 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

While kfree_rcu(ptr) _is_ supported, it has some limitations.

Given that 99.99% of kfree_rcu() users [1] use the legacy
two parameters variant, and @catchall objects do have an rcu head,
simply use it.

Choice of kfree_rcu(ptr) variant was probably not intentional.

[1] including calls from net/netfilter/nf_tables_api.c

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Florian Westphal Feb. 22, 2022, 7:46 p.m. UTC | #1 
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> While kfree_rcu(ptr) _is_ supported, it has some limitations.
> 
> Given that 99.99% of kfree_rcu() users [1] use the legacy
> two parameters variant, and @catchall objects do have an rcu head,
> simply use it.
> 
> Choice of kfree_rcu(ptr) variant was probably not intentional.

In case someone wondered, this causes expensive
sycnhronize_rcu + kfree for each removal operation.

Reviewed-by: Florian Westphal <fw@strlen.de>
Eric Dumazet Feb. 22, 2022, 8:07 p.m. UTC | #2 
On Tue, Feb 22, 2022 at 11:46 AM Florian Westphal <fw@strlen.de> wrote:
>
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > While kfree_rcu(ptr) _is_ supported, it has some limitations.
> >
> > Given that 99.99% of kfree_rcu() users [1] use the legacy
> > two parameters variant, and @catchall objects do have an rcu head,
> > simply use it.
> >
> > Choice of kfree_rcu(ptr) variant was probably not intentional.
>
> In case someone wondered, this causes expensive
> sycnhronize_rcu + kfree for each removal operation.

This fallback to synchronize_rcu() only happens if kvfree_call_rcu() has been
unable to allocate a new block of memory.

But yes, I guess I would add a Fixes: tag, because we can easily avoid
this potential issue.

Pablo, if not too late:

Fixes: aaa31047a6d2 ("netfilter: nftables: add catch-all set element support")

>
> Reviewed-by: Florian Westphal <fw@strlen.de>
>
Pablo Neira Ayuso Feb. 23, 2022, 8:22 a.m. UTC | #3 
On Tue, Feb 22, 2022 at 12:07:05PM -0800, Eric Dumazet wrote:
> On Tue, Feb 22, 2022 at 11:46 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > While kfree_rcu(ptr) _is_ supported, it has some limitations.
> > >
> > > Given that 99.99% of kfree_rcu() users [1] use the legacy
> > > two parameters variant, and @catchall objects do have an rcu head,
> > > simply use it.
> > >
> > > Choice of kfree_rcu(ptr) variant was probably not intentional.
> >
> > In case someone wondered, this causes expensive
> > sycnhronize_rcu + kfree for each removal operation.
> 
> This fallback to synchronize_rcu() only happens if kvfree_call_rcu() has been
> unable to allocate a new block of memory.
> 
> But yes, I guess I would add a Fixes: tag, because we can easily avoid
> this potential issue.
> 
> Pablo, if not too late:
> 
> Fixes: aaa31047a6d2 ("netfilter: nftables: add catch-all set element support")

Applied, thanks!
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 5fa16990da95177791dfaa9e7bb31c92f3cae096..ef79d8d09773588cc399113fd5bcf584c592f039 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4502,7 +4502,7 @@  static void nft_set_catchall_destroy(const struct nft_ctx *ctx,
 	list_for_each_entry_safe(catchall, next, &set->catchall_list, list) {
 		list_del_rcu(&catchall->list);
 		nft_set_elem_destroy(set, catchall->elem, true);
-		kfree_rcu(catchall);
+		kfree_rcu(catchall, rcu);
 	}
 }
 
@@ -5669,7 +5669,7 @@  static void nft_setelem_catchall_remove(const struct net *net,
 	list_for_each_entry_safe(catchall, next, &set->catchall_list, list) {
 		if (catchall->elem == elem->priv) {
 			list_del_rcu(&catchall->list);
-			kfree_rcu(catchall);
+			kfree_rcu(catchall, rcu);
 			break;
 		}
 	}