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

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

[Eric Dumazet]

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(-)

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;
 		}
 	}
-- 
2.35.1.473.g83b2b277ed-goog

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

[Florian Westphal]

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>

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

[Eric Dumazet]

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>
>

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

[Pablo Neira Ayuso]

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!