* [PATCH 6.6.y 1/2] netfilter: nf_tables: remove catchall element in GC sync path
2023-11-21 12:14 [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free Florian Westphal
@ 2023-11-21 12:14 ` Florian Westphal
2023-11-21 12:14 ` [PATCH 6.6.y 2/2] netfilter: nf_tables: split async and sync catchall in two functions Florian Westphal
2023-11-21 21:39 ` [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free Pablo Neira Ayuso
2 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2023-11-21 12:14 UTC (permalink / raw)
To: stable; +Cc: netfilter-devel, Pablo Neira Ayuso, lonial con, Florian Westphal
From: Pablo Neira Ayuso <pablo@netfilter.org>
[ Upstream commit 93995bf4af2c5a99e2a87f0cd5ce547d31eb7630 ]
The expired catchall element is not deactivated and removed from GC sync
path. This path holds mutex so just call nft_setelem_data_deactivate()
and nft_setelem_catchall_remove() before queueing the GC work.
Fixes: 4a9e12ea7e70 ("netfilter: nft_set_pipapo: call nft_trans_gc_queue_sync() in catchall GC")
Reported-by: lonial con <kongln9170@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/netfilter/nf_tables_api.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3bf428a188cc..d300e9fc5d62 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6464,6 +6464,12 @@ static int nft_setelem_deactivate(const struct net *net,
return ret;
}
+static void nft_setelem_catchall_destroy(struct nft_set_elem_catchall *catchall)
+{
+ list_del_rcu(&catchall->list);
+ kfree_rcu(catchall, rcu);
+}
+
static void nft_setelem_catchall_remove(const struct net *net,
const struct nft_set *set,
const struct nft_set_elem *elem)
@@ -6472,8 +6478,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, rcu);
+ nft_setelem_catchall_destroy(catchall);
break;
}
}
@@ -9638,11 +9643,12 @@ static struct nft_trans_gc *nft_trans_gc_catchall(struct nft_trans_gc *gc,
unsigned int gc_seq,
bool sync)
{
- struct nft_set_elem_catchall *catchall;
+ struct nft_set_elem_catchall *catchall, *next;
const struct nft_set *set = gc->set;
+ struct nft_elem_priv *elem_priv;
struct nft_set_ext *ext;
- list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
+ list_for_each_entry_safe(catchall, next, &set->catchall_list, list) {
ext = nft_set_elem_ext(set, catchall->elem);
if (!nft_set_elem_expired(ext))
@@ -9660,7 +9666,17 @@ static struct nft_trans_gc *nft_trans_gc_catchall(struct nft_trans_gc *gc,
if (!gc)
return NULL;
- nft_trans_gc_elem_add(gc, catchall->elem);
+ elem_priv = catchall->elem;
+ if (sync) {
+ struct nft_set_elem elem = {
+ .priv = elem_priv,
+ };
+
+ nft_setelem_data_deactivate(gc->net, gc->set, &elem);
+ nft_setelem_catchall_destroy(catchall);
+ }
+
+ nft_trans_gc_elem_add(gc, elem_priv);
}
return gc;
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 6.6.y 2/2] netfilter: nf_tables: split async and sync catchall in two functions
2023-11-21 12:14 [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free Florian Westphal
2023-11-21 12:14 ` [PATCH 6.6.y 1/2] netfilter: nf_tables: remove catchall element in GC sync path Florian Westphal
@ 2023-11-21 12:14 ` Florian Westphal
2023-11-21 21:39 ` [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free Pablo Neira Ayuso
2 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2023-11-21 12:14 UTC (permalink / raw)
To: stable; +Cc: netfilter-devel, Pablo Neira Ayuso, Florian Westphal
From: Pablo Neira Ayuso <pablo@netfilter.org>
[ Upstream commit 8837ba3e58ea1e3d09ae36db80b1e80853aada95 ]
list_for_each_entry_safe() does not work for the async case which runs
under RCU, therefore, split GC logic for catchall in two functions
instead, one for each of the sync and async GC variants.
The catchall sync GC variant never sees a _DEAD bit set on ever, thus,
this handling is removed in such case, moreover, allocate GC sync batch
via GFP_KERNEL.
Fixes: 93995bf4af2c ("netfilter: nf_tables: remove catchall element in GC sync path")
Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/netfilter/nf_tables_api.c | 61 ++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d300e9fc5d62..f51876bc0947 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9639,16 +9639,14 @@ void nft_trans_gc_queue_sync_done(struct nft_trans_gc *trans)
call_rcu(&trans->rcu, nft_trans_gc_trans_free);
}
-static struct nft_trans_gc *nft_trans_gc_catchall(struct nft_trans_gc *gc,
- unsigned int gc_seq,
- bool sync)
+struct nft_trans_gc *nft_trans_gc_catchall_async(struct nft_trans_gc *gc,
+ unsigned int gc_seq)
{
- struct nft_set_elem_catchall *catchall, *next;
+ struct nft_set_elem_catchall *catchall;
const struct nft_set *set = gc->set;
- struct nft_elem_priv *elem_priv;
struct nft_set_ext *ext;
- list_for_each_entry_safe(catchall, next, &set->catchall_list, list) {
+ list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
ext = nft_set_elem_ext(set, catchall->elem);
if (!nft_set_elem_expired(ext))
@@ -9658,39 +9656,44 @@ static struct nft_trans_gc *nft_trans_gc_catchall(struct nft_trans_gc *gc,
nft_set_elem_dead(ext);
dead_elem:
- if (sync)
- gc = nft_trans_gc_queue_sync(gc, GFP_ATOMIC);
- else
- gc = nft_trans_gc_queue_async(gc, gc_seq, GFP_ATOMIC);
-
+ gc = nft_trans_gc_queue_async(gc, gc_seq, GFP_ATOMIC);
if (!gc)
return NULL;
- elem_priv = catchall->elem;
- if (sync) {
- struct nft_set_elem elem = {
- .priv = elem_priv,
- };
-
- nft_setelem_data_deactivate(gc->net, gc->set, &elem);
- nft_setelem_catchall_destroy(catchall);
- }
-
- nft_trans_gc_elem_add(gc, elem_priv);
+ nft_trans_gc_elem_add(gc, catchall->elem);
}
return gc;
}
-struct nft_trans_gc *nft_trans_gc_catchall_async(struct nft_trans_gc *gc,
- unsigned int gc_seq)
-{
- return nft_trans_gc_catchall(gc, gc_seq, false);
-}
-
struct nft_trans_gc *nft_trans_gc_catchall_sync(struct nft_trans_gc *gc)
{
- return nft_trans_gc_catchall(gc, 0, true);
+ struct nft_set_elem_catchall *catchall, *next;
+ const struct nft_set *set = gc->set;
+ struct nft_set_elem elem;
+ struct nft_set_ext *ext;
+
+ WARN_ON_ONCE(!lockdep_commit_lock_is_held(gc->net));
+
+ list_for_each_entry_safe(catchall, next, &set->catchall_list, list) {
+ ext = nft_set_elem_ext(set, catchall->elem);
+
+ if (!nft_set_elem_expired(ext))
+ continue;
+
+ gc = nft_trans_gc_queue_sync(gc, GFP_KERNEL);
+ if (!gc)
+ return NULL;
+
+ memset(&elem, 0, sizeof(elem));
+ elem.priv = catchall->elem;
+
+ nft_setelem_data_deactivate(gc->net, gc->set, &elem);
+ nft_setelem_catchall_destroy(catchall);
+ nft_trans_gc_elem_add(gc, elem.priv);
+ }
+
+ return gc;
}
static void nf_tables_module_autoload_cleanup(struct net *net)
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free
2023-11-21 12:14 [PATCH 6.6.y 0/2] netfilter: fix catchall element double-free Florian Westphal
2023-11-21 12:14 ` [PATCH 6.6.y 1/2] netfilter: nf_tables: remove catchall element in GC sync path Florian Westphal
2023-11-21 12:14 ` [PATCH 6.6.y 2/2] netfilter: nf_tables: split async and sync catchall in two functions Florian Westphal
@ 2023-11-21 21:39 ` Pablo Neira Ayuso
2023-11-22 21:18 ` Sasha Levin
2 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2023-11-21 21:39 UTC (permalink / raw)
To: Greg Kroah-Hartman, Sasha Levin; +Cc: Florian Westphal, stable, netfilter-devel
Hi Greg, Sasha,
On Tue, Nov 21, 2023 at 01:14:20PM +0100, Florian Westphal wrote:
> Hello,
>
> This series contains the backports of two related changes to fix
> removal of timed-out catchall elements.
>
> As-is, removed element remains on the list and will be collected
> again.
>
> The adjustments are needed because of missing commit
> 0e1ea651c971 ("netfilter: nf_tables: shrink memory consumption of set elements"),
> so we need to pass set_elem container struct instead of "elem_priv".
Please, also apply this series to -stable 5.15, 6.1 and 6.5.
This series apply cleanly to these -stable kernels, I have also tested
this series on them.
Tested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread