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".
Pablo Neira Ayuso (2): netfilter: nf_tables: remove catchall element in GC sync path netfilter: nf_tables: split async and sync catchall in two functions
net/netfilter/nf_tables_api.c | 53 ++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 17 deletions(-)
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;
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)
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH 2/2] netfilter: nf_tables: split async and sync catchall in two functions Link: https://lore.kernel.org/stable/20231121121431.8612-3-fw%40strlen.de
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.
On Tue, Nov 21, 2023 at 10:39:54PM +0100, Pablo Neira Ayuso wrote:
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
Queued up, thanks!
linux-stable-mirror@lists.linaro.org
-
Florian Westphal -
kernel test robot -
Pablo Neira Ayuso -
Sasha Levin