Skip to contentSkip to conversation

Netfilter fixes for net

PATCH Applied Pablo Neira Ayuso 5 participants 11 messages Aug 10, 2023

Subsystems (4)
Networking Network drivers Stable branch netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

cover Netfilter fixes for net

Hi,

The following patchset contains Netfilter fixes for net.

The existing attempt to resolve races between control plane and GC work is error prone, as reported by Bien Pham <[email protected]>, some places forgot to call nft_set_elem_mark_busy(), leading to double-deactivation of elements.

This series contains the following patches:

1) Do not skip expired elements during walk otherwise elements might never decrement the reference counter on data, leading to memleak.

2) Add a GC transaction API to replace the former attempt to deal with races between control plane and GC. GC worker sets on NFT_SET_ELEM_DEAD_BIT on elements and it creates a GC transaction to remove the expired elements, GC transaction could abort in case of interference with control plane and retried later (GC async). Set backends such as rbtree and pipapo also perform GC from control plane (GC sync), in such case, element deactivation and removal is safe because mutex is held then collected elements are released via call_rcu().

3) Adapt existing set backends to use the GC transaction API.

4) Update rhash set backend to set on _DEAD bit to report deleted elements from datapath for GC.

5) Remove old GC batch API and the NFT_SET_ELEM_BUSY_BIT.

Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

Please, pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-23-08-10

Thanks.

----------------------------------------------------------------

The following changes since commit c5ccff70501d92db445a135fa49cf9bc6b98c444:

Merge branch 'net-sched-bind-logic-fixes-for-cls_fw-cls_u32-and-cls_route' (2023-07-31 20:10:39 -0700)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-23-08-10

for you to fetch changes up to a2dd0233cbc4d8a0abb5f64487487ffc9265beb5:

netfilter: nf_tables: remove busy mark and gc batch API (2023-08-10 08:25:27 +0200)

----------------------------------------------------------------
netfilter pull request 23-08-10

----------------------------------------------------------------
Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

 include/net/netfilter/nf_tables.h | 120 ++++++---------
 net/netfilter/nf_tables_api.c     | 307 ++++++++++++++++++++++++++++++--------
 net/netfilter/nft_set_hash.c      |  85 +++++++----
 net/netfilter/nft_set_pipapo.c    |  66 +++++---
 net/netfilter/nft_set_rbtree.c    | 146 ++++++++++--------
 5 files changed, 476 insertions(+), 248 deletions(-)
author maintainer
#1 lore raw

PATCH 1/5 netfilter: nf_tables: don't skip expired elements during walk

From: Florian Westphal <[email protected]>

There is an asymmetry between commit/abort and preparation phase if the following conditions are met:

1. set is a verdict map ("1.2.3.4 : jump foo")
2. timeouts are enabled

In this case, following sequence is problematic:

1. element E in set S refers to chain C
2. userspace requests removal of set S
3. kernel does a set walk to decrement chain->use count for all elements from preparation phase
4. kernel does another set walk to remove elements from the commit phase (or another walk to do a chain->use increment for all elements from abort phase)

If E has already expired in 1), it will be ignored during list walk, so its use count won't have been changed.

Then, when set is culled, ->destroy callback will zap the element via nf_tables_set_elem_destroy(), but this function is only safe for elements that have been deactivated earlier from the preparation phase:
lack of earlier deactivate removes the element but leaks the chain use count, which results in a WARN splat when the chain gets removed later, plus a leak of the nft_chain structure.

Update pipapo_get() not to skip expired elements, otherwise flush command reports bogus ENOENT errors.

Fixes 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
Fixes 8d8540c4f5e0 ("netfilter: nft_set_rbtree: add timeout support")
Fixes 9d0982927e79 ("netfilter: nft_hash: add support for timeouts")
Signed-off-by Florian Westphal <[email protected]>
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_tables_api.c  |  4 ++++
 net/netfilter/nft_set_hash.c   |  2 --
 net/netfilter/nft_set_pipapo.c | 18 ++++++++++++------
 net/netfilter/nft_set_rbtree.c |  2 --
 4 files changed, 16 insertions(+), 10 deletions(-)
net/netfilter/nf_tables_api.c +4 -0
index d3c6ecd1f5a6..b4321869e5c6 100644
@@ -5602,8 +5602,12 @@ static int nf_tables_dump_setelem(const struct nft_ctx *ctx,
 				  const struct nft_set_iter *iter,
 				  struct nft_set_elem *elem)
 {
+	const struct nft_set_ext *ext = nft_set_elem_ext(set, elem->priv);
 	struct nft_set_dump_args *args;
 
+	if (nft_set_elem_expired(ext))
+		return 0;
+
 	args = container_of(iter, struct nft_set_dump_args, iter);
 	return nf_tables_fill_setelem(args->skb, set, elem, args->reset);
 }
net/netfilter/nft_set_hash.c +0 -2
index 0b73cb0e752f..24caa31fa231 100644
@@ -278,8 +278,6 @@ static void nft_rhash_walk(const struct nft_ctx *ctx, struct nft_set *set,
 
 		if (iter->count < iter->skip)
 			goto cont;
-		if (nft_set_elem_expired(&he->ext))
-			goto cont;
 		if (!nft_set_elem_active(&he->ext, iter->genmask))
 			goto cont;
net/netfilter/nft_set_pipapo.c +12 -6
index 49915a2a58eb..d54784ea465b 100644
@@ -566,8 +566,7 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net,
 			goto out;
 
 		if (last) {
-			if (nft_set_elem_expired(&f->mt[b].e->ext) ||
-			    (genmask &&
+			if ((genmask &&
 			     !nft_set_elem_active(&f->mt[b].e->ext, genmask)))
 				goto next_match;
 

@@ -601,8 +600,17 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net,
 static void *nft_pipapo_get(const struct net *net, const struct nft_set *set,
 			    const struct nft_set_elem *elem, unsigned int flags)
 {
-	return pipapo_get(net, set, (const u8 *)elem->key.val.data,
-			  nft_genmask_cur(net));
+	struct nft_pipapo_elem *ret;
+
+	ret = pipapo_get(net, set, (const u8 *)elem->key.val.data,
+			 nft_genmask_cur(net));
+	if (IS_ERR(ret))
+		return ret;
+
+	if (nft_set_elem_expired(&ret->ext))
+		return ERR_PTR(-ENOENT);
+
+	return ret;
 }
 
 /**

@@ -2005,8 +2013,6 @@ static void nft_pipapo_walk(const struct nft_ctx *ctx, struct nft_set *set,
 			goto cont;
 
 		e = f->mt[r].e;
-		if (nft_set_elem_expired(&e->ext))
-			goto cont;
 
 		elem.priv = e;
net/netfilter/nft_set_rbtree.c +0 -2
index 8d73fffd2d09..39956e5341c9 100644
@@ -552,8 +552,6 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 
 		if (iter->count < iter->skip)
 			goto cont;
-		if (nft_set_elem_expired(&rbe->ext))
-			goto cont;
 		if (!nft_set_elem_active(&rbe->ext, iter->genmask))
 			goto cont;
Signature · 1 line
2.30.2
build bot
#2 lore raw

Re: [PATCH net 1/5] netfilter: nf_tables: don't skip expired elements during walk

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Rule: 'Cc: [email protected]' or 'commit <sha1> upstream.'
Subject: [PATCH net 1/5] netfilter: nf_tables: don't skip expired elements during walk

Link https://lore.kernel.org/stable/20230810070830.24064-2-pablo%40netfilter.org open here

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
CI Applied
#3 lore raw

Re: [PATCH net 1/5] netfilter: nf_tables: don't skip expired elements during walk

In reply to Pablo Neira Ayuso
This series was applied to netdev/net.git (main) by Pablo Neira Ayuso <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Pablo Neira Ayuso <[email protected]>:

Quoted 9 lines · On Thu, 10 Aug 2023 09:08:26 +0200 you wrote:

From: Florian Westphal <[email protected]>

There is an asymmetry between commit/abort and preparation phase if the following conditions are met:

1. set is a verdict map ("1.2.3.4 : jump foo")
2. timeouts are enabled

[...]

Here is the summary with links:
- [net,1/5] netfilter: nf_tables: don't skip expired elements during walk

    https://git.kernel.org/netdev/net/c/24138933b97b
  - [net,2/5] netfilter: nf_tables: GC transaction API to avoid race with control plane
    https://git.kernel.org/netdev/net/c/5f68718b34a5
  - [net,3/5] netfilter: nf_tables: adapt set backend to use GC transaction API
    https://git.kernel.org/netdev/net/c/f6c383b8c31a
  - [net,4/5] netfilter: nft_set_hash: mark set element as dead when deleting from packet path
    https://git.kernel.org/netdev/net/c/c92db3030492
  - [net,5/5] netfilter: nf_tables: remove busy mark and gc batch API
    https://git.kernel.org/netdev/net/c/a2dd0233cbc4

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
author maintainer
#4 lore raw

PATCH 2/5 netfilter: nf_tables: GC transaction API to avoid race with control plane

In reply to Pablo Neira Ayuso

The set types rhashtable and rbtree use a GC worker to reclaim memory. From system work queue, in periodic intervals, a scan of the table is done.

The major caveat here is that the nft transaction mutex is not held. This causes a race between control plane and GC when they attempt to delete the same element.

We cannot grab the netlink mutex from the work queue, because the control plane has to wait for the GC work queue in case the set is to be removed, so we get following deadlock:

cpu 1 cpu2
GC work transaction comes in , lock nft mutex
`acquire nft mutex // BLOCKS
transaction asks to remove the set set destruction calls cancel_work_sync()

cancel_work_sync will now block forever, because it is waiting for the mutex the caller already owns.

This patch adds a new API that deals with garbage collection in two steps:

1) Lockless GC of expired elements sets on the NFT_SET_ELEM_DEAD_BIT so they are not visible via lookup. Annotate current GC sequence in the GC transaction. Enqueue GC transaction work as soon as it is full. If ruleset is updated, then GC transaction is aborted and retried later.

2) GC work grabs the mutex. If GC sequence has changed then this GC transaction lost race with control plane, abort it as it contains stale references to objects and let GC try again later. If the ruleset is intact, then this GC transaction deactivates and removes the elements and it uses call_rcu() to destroy elements.

Note that no elements are removed from GC lockless path, the _DEAD bit is set and pointers are collected. GC catchall does not remove the elements anymore too. There is a new set->dead flag that is set on to abort the GC transaction to deal with set->ops->destroy() path which removes the remaining elements in the set from commit_release, where no mutex is held.

To deal with GC when mutex is held, which allows safe deactivate and removal, add sync GC API which releases the set element object via call_rcu(). This is used by rbtree and pipapo backends which also perform garbage collection from control plane path.

Since element removal from sets can happen from control plane and element garbage collection/timeout, it is necessary to keep the set structure alive until all elements have been deactivated and destroyed.

We cannot do a cancel_work_sync or flush_work in nft_set_destroy because its called with the transaction mutex held, but the aforementioned async work queue might be blocked on the very mutex that nft_set_destroy() callchain is sitting on.

This gives us the choice of ABBA deadlock or UaF.

To avoid both, add set->refs refcount_t member. The GC API can then increment the set refcount and release it once the elements have been free'd.

Set backends are adapted to use the GC transaction API in a follow up patch entitled:

("netfilter: nf_tables: use gc transaction API in set backends")

This is joint work with Florian Westphal.

Fixes cfed7e1b1f8e ("netfilter: nf_tables: add set garbage collection helpers")
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 include/net/netfilter/nf_tables.h |  64 +++++++-
 net/netfilter/nf_tables_api.c     | 248 ++++++++++++++++++++++++++++--
 2 files changed, 300 insertions(+), 12 deletions(-)
include/net/netfilter/nf_tables.h +63 -1
index 640441a2f926..7256e9c80477 100644
@@ -512,6 +512,7 @@ struct nft_set_elem_expr {
  *
  *	@list: table set list node
  *	@bindings: list of set bindings
+ *	@refs: internal refcounting for async set destruction
  *	@table: table this set belongs to
  *	@net: netnamespace this set belongs to
  * 	@name: name of the set

@@ -541,6 +542,7 @@ struct nft_set_elem_expr {
 struct nft_set {
 	struct list_head		list;
 	struct list_head		bindings;
+	refcount_t			refs;
 	struct nft_table		*table;
 	possible_net_t			net;
 	char				*name;

@@ -562,7 +564,8 @@ struct nft_set {
 	struct list_head		pending_update;
 	/* runtime data below here */
 	const struct nft_set_ops	*ops ____cacheline_aligned;
-	u16				flags:14,
+	u16				flags:13,
+					dead:1,
 					genmask:2;
 	u8				klen;
 	u8				dlen;

@@ -1592,6 +1595,32 @@ static inline void nft_set_elem_clear_busy(struct nft_set_ext *ext)
 	clear_bit(NFT_SET_ELEM_BUSY_BIT, word);
 }
 
+#define NFT_SET_ELEM_DEAD_MASK	(1 << 3)
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+#define NFT_SET_ELEM_DEAD_BIT	3
+#elif defined(__BIG_ENDIAN_BITFIELD)
+#define NFT_SET_ELEM_DEAD_BIT	(BITS_PER_LONG - BITS_PER_BYTE + 3)
+#else
+#error
+#endif
+
+static inline void nft_set_elem_dead(struct nft_set_ext *ext)
+{
+	unsigned long *word = (unsigned long *)ext;
+
+	BUILD_BUG_ON(offsetof(struct nft_set_ext, genmask) != 0);
+	set_bit(NFT_SET_ELEM_DEAD_BIT, word);
+}
+
+static inline int nft_set_elem_is_dead(const struct nft_set_ext *ext)
+{
+	unsigned long *word = (unsigned long *)ext;
+
+	BUILD_BUG_ON(offsetof(struct nft_set_ext, genmask) != 0);
+	return test_bit(NFT_SET_ELEM_DEAD_BIT, word);
+}
+
 /**
  *	struct nft_trans - nf_tables object update in transaction
  *

@@ -1732,6 +1761,38 @@ struct nft_trans_flowtable {
 #define nft_trans_flowtable_flags(trans)	\
 	(((struct nft_trans_flowtable *)trans->data)->flags)
 
+#define NFT_TRANS_GC_BATCHCOUNT	256
+
+struct nft_trans_gc {
+	struct list_head	list;
+	struct net		*net;
+	struct nft_set		*set;
+	u32			seq;
+	u8			count;
+	void			*priv[NFT_TRANS_GC_BATCHCOUNT];
+	struct rcu_head		rcu;
+};
+
+struct nft_trans_gc *nft_trans_gc_alloc(struct nft_set *set,
+					unsigned int gc_seq, gfp_t gfp);
+void nft_trans_gc_destroy(struct nft_trans_gc *trans);
+
+struct nft_trans_gc *nft_trans_gc_queue_async(struct nft_trans_gc *gc,
+					      unsigned int gc_seq, gfp_t gfp);
+void nft_trans_gc_queue_async_done(struct nft_trans_gc *gc);
+
+struct nft_trans_gc *nft_trans_gc_queue_sync(struct nft_trans_gc *gc, gfp_t gfp);
+void nft_trans_gc_queue_sync_done(struct nft_trans_gc *trans);
+
+void nft_trans_gc_elem_add(struct nft_trans_gc *gc, void *priv);
+
+struct nft_trans_gc *nft_trans_gc_catchall(struct nft_trans_gc *gc,
+					   unsigned int gc_seq);
+
+void nft_setelem_data_deactivate(const struct net *net,
+				 const struct nft_set *set,
+				 struct nft_set_elem *elem);
+
 int __init nft_chain_filter_init(void);
 void nft_chain_filter_fini(void);
 

@@ -1758,6 +1819,7 @@ struct nftables_pernet {
 	struct mutex		commit_mutex;
 	u64			table_handle;
 	unsigned int		base_seq;
+	unsigned int		gc_seq;
 };
 
 extern unsigned int nf_tables_net_id;
net/netfilter/nf_tables_api.c +237 -11

353 lines not on this page · raw message

Signature · 1 line
2.30.2
author maintainer
#5 lore raw

PATCH 3/5 netfilter: nf_tables: adapt set backend to use GC transaction API

In reply to Pablo Neira Ayuso

Use the GC transaction API to replace the old and buggy gc API and the busy mark approach.

No set elements are removed from async garbage collection anymore, instead the _DEAD bit is set on so the set element is not visible from lookup path anymore. Async GC enqueues transaction work that might be aborted and retried later.

rbtree and pipapo set backends does not set on the _DEAD bit from the sync GC path since this runs in control plane path where mutex is held. In this case, set elements are deactivated, removed and then released via RCU callback, sync GC never fails.

Fixes 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
Fixes 8d8540c4f5e0 ("netfilter: nft_set_rbtree: add timeout support")
Fixes 9d0982927e79 ("netfilter: nft_hash: add support for timeouts")
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_tables_api.c  |   7 +-
 net/netfilter/nft_set_hash.c   |  77 +++++++++++-------
 net/netfilter/nft_set_pipapo.c |  48 ++++++++---
 net/netfilter/nft_set_rbtree.c | 144 ++++++++++++++++++++-------------
 4 files changed, 173 insertions(+), 103 deletions(-)
net/netfilter/nf_tables_api.c +2 -5
index c28bacb9479b..fd4b5da7ac3c 100644
@@ -6380,7 +6380,6 @@ static void nft_setelem_activate(struct net *net, struct nft_set *set,
 
 	if (nft_setelem_is_catchall(set, elem)) {
 		nft_set_elem_change_active(net, set, ext);
-		nft_set_elem_clear_busy(ext);
 	} else {
 		set->ops->activate(net, set, elem);
 	}

@@ -6395,8 +6394,7 @@ static int nft_setelem_catchall_deactivate(const struct net *net,
 
 	list_for_each_entry(catchall, &set->catchall_list, list) {
 		ext = nft_set_elem_ext(set, catchall->elem);
-		if (!nft_is_active(net, ext) ||
-		    nft_set_elem_mark_busy(ext))
+		if (!nft_is_active(net, ext))
 			continue;
 
 		kfree(elem->priv);

@@ -7109,8 +7107,7 @@ static int nft_set_catchall_flush(const struct nft_ctx *ctx,
 
 	list_for_each_entry_rcu(catchall, &set->catchall_list, list) {
 		ext = nft_set_elem_ext(set, catchall->elem);
-		if (!nft_set_elem_active(ext, genmask) ||
-		    nft_set_elem_mark_busy(ext))
+		if (!nft_set_elem_active(ext, genmask))
 			continue;
 
 		elem.priv = catchall->elem;
net/netfilter/nft_set_hash.c +48 -29

140 lines not on this page · raw message

net/netfilter/nft_set_pipapo.c +36 -12

83 lines not on this page · raw message

net/netfilter/nft_set_rbtree.c +87 -57

232 lines not on this page · raw message

Signature · 1 line
2.30.2
author maintainer
#6 lore raw

PATCH 4/5 netfilter: nft_set_hash: mark set element as dead when deleting from packet path

In reply to Pablo Neira Ayuso

Set on the NFT_SET_ELEM_DEAD_BIT flag on this element, instead of performing element removal which might race with an ongoing transaction. Enable gc when dynamic flag is set on since dynset deletion requires garbage collection after this patch.

Fixes d0a8d877da97 ("netfilter: nft_dynset: support for element deletion")
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nft_set_hash.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
net/netfilter/nft_set_hash.c +4 -2
index 2f067e4596b0..cef5df846000 100644
@@ -249,7 +249,9 @@ static bool nft_rhash_delete(const struct nft_set *set,
 	if (he == NULL)
 		return false;
 
-	return rhashtable_remove_fast(&priv->ht, &he->node, nft_rhash_params) == 0;
+	nft_set_elem_dead(&he->ext);
+
+	return true;
 }
 
 static void nft_rhash_walk(const struct nft_ctx *ctx, struct nft_set *set,

@@ -412,7 +414,7 @@ static int nft_rhash_init(const struct nft_set *set,
 		return err;
 
 	INIT_DEFERRABLE_WORK(&priv->gc_work, nft_rhash_gc);
-	if (set->flags & NFT_SET_TIMEOUT)
+	if (set->flags & (NFT_SET_TIMEOUT | NFT_SET_EVAL))
 		nft_rhash_gc_init(set);
 
 	return 0;
Signature · 1 line
2.30.2
author maintainer
#7 lore raw

PATCH 5/5 netfilter: nf_tables: remove busy mark and gc batch API

In reply to Pablo Neira Ayuso

Ditch it, it has been replace it by the GC transaction API and it has no clients anymore.

Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 include/net/netfilter/nf_tables.h | 98 +------------------------------
 net/netfilter/nf_tables_api.c     | 48 +--------------
 2 files changed, 4 insertions(+), 142 deletions(-)
include/net/netfilter/nf_tables.h +3 -95
index 7256e9c80477..35870858ddf2 100644
@@ -599,7 +599,6 @@ struct nft_set *nft_set_lookup_global(const struct net *net,
 
 struct nft_set_ext *nft_set_catchall_lookup(const struct net *net,
 					    const struct nft_set *set);
-void *nft_set_catchall_gc(const struct nft_set *set);
 
 static inline unsigned long nft_set_gc_interval(const struct nft_set *set)
 {

@@ -816,62 +815,6 @@ void nft_set_elem_destroy(const struct nft_set *set, void *elem,
 void nf_tables_set_elem_destroy(const struct nft_ctx *ctx,
 				const struct nft_set *set, void *elem);
 
-/**
- *	struct nft_set_gc_batch_head - nf_tables set garbage collection batch
- *
- *	@rcu: rcu head
- *	@set: set the elements belong to
- *	@cnt: count of elements
- */
-struct nft_set_gc_batch_head {
-	struct rcu_head			rcu;
-	const struct nft_set		*set;
-	unsigned int			cnt;
-};
-
-#define NFT_SET_GC_BATCH_SIZE	((PAGE_SIZE -				  \
-				  sizeof(struct nft_set_gc_batch_head)) / \
-				 sizeof(void *))
-
-/**
- *	struct nft_set_gc_batch - nf_tables set garbage collection batch
- *
- * 	@head: GC batch head
- * 	@elems: garbage collection elements
- */
-struct nft_set_gc_batch {
-	struct nft_set_gc_batch_head	head;
-	void				*elems[NFT_SET_GC_BATCH_SIZE];
-};
-
-struct nft_set_gc_batch *nft_set_gc_batch_alloc(const struct nft_set *set,
-						gfp_t gfp);
-void nft_set_gc_batch_release(struct rcu_head *rcu);
-
-static inline void nft_set_gc_batch_complete(struct nft_set_gc_batch *gcb)
-{
-	if (gcb != NULL)
-		call_rcu(&gcb->head.rcu, nft_set_gc_batch_release);
-}
-
-static inline struct nft_set_gc_batch *
-nft_set_gc_batch_check(const struct nft_set *set, struct nft_set_gc_batch *gcb,
-		       gfp_t gfp)
-{
-	if (gcb != NULL) {
-		if (gcb->head.cnt + 1 < ARRAY_SIZE(gcb->elems))
-			return gcb;
-		nft_set_gc_batch_complete(gcb);
-	}
-	return nft_set_gc_batch_alloc(set, gfp);
-}
-
-static inline void nft_set_gc_batch_add(struct nft_set_gc_batch *gcb,
-					void *elem)
-{
-	gcb->elems[gcb->head.cnt++] = elem;
-}
-
 struct nft_expr_ops;
 /**
  *	struct nft_expr_type - nf_tables expression type

@@ -1560,47 +1503,12 @@ static inline void nft_set_elem_change_active(const struct net *net,
 
 #endif /* IS_ENABLED(CONFIG_NF_TABLES) */
 
-/*
- * We use a free bit in the genmask field to indicate the element
- * is busy, meaning it is currently being processed either by
- * the netlink API or GC.
- *
- * Even though the genmask is only a single byte wide, this works
- * because the extension structure if fully constant once initialized,
- * so there are no non-atomic write accesses unless it is already
- * marked busy.
- */
-#define NFT_SET_ELEM_BUSY_MASK	(1 << 2)
-
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-#define NFT_SET_ELEM_BUSY_BIT	2
-#elif defined(__BIG_ENDIAN_BITFIELD)
-#define NFT_SET_ELEM_BUSY_BIT	(BITS_PER_LONG - BITS_PER_BYTE + 2)
-#else
-#error
-#endif
-
-static inline int nft_set_elem_mark_busy(struct nft_set_ext *ext)
-{
-	unsigned long *word = (unsigned long *)ext;
-
-	BUILD_BUG_ON(offsetof(struct nft_set_ext, genmask) != 0);
-	return test_and_set_bit(NFT_SET_ELEM_BUSY_BIT, word);
-}
-
-static inline void nft_set_elem_clear_busy(struct nft_set_ext *ext)
-{
-	unsigned long *word = (unsigned long *)ext;
-
-	clear_bit(NFT_SET_ELEM_BUSY_BIT, word);
-}
-
-#define NFT_SET_ELEM_DEAD_MASK	(1 << 3)
+#define NFT_SET_ELEM_DEAD_MASK	(1 << 2)
 
 #if defined(__LITTLE_ENDIAN_BITFIELD)
-#define NFT_SET_ELEM_DEAD_BIT	3
+#define NFT_SET_ELEM_DEAD_BIT	2
 #elif defined(__BIG_ENDIAN_BITFIELD)
-#define NFT_SET_ELEM_DEAD_BIT	(BITS_PER_LONG - BITS_PER_BYTE + 3)
+#define NFT_SET_ELEM_DEAD_BIT	(BITS_PER_LONG - BITS_PER_BYTE + 2)
 #else
 #error
 #endif
net/netfilter/nf_tables_api.c +1 -47

66 lines not on this page · raw message

Signature · 1 line
2.30.2
maintainer
#8 lore raw
In reply to Pablo Neira Ayuso
Quoted 72 lines · Pablo Neira Ayuso

Hi,

The following patchset contains Netfilter fixes for net.

The existing attempt to resolve races between control plane and GC work is error prone, as reported by Bien Pham <[email protected]>, some places forgot to call nft_set_elem_mark_busy(), leading to double-deactivation of elements.

This series contains the following patches:

1) Do not skip expired elements during walk otherwise elements might never decrement the reference counter on data, leading to memleak.

2) Add a GC transaction API to replace the former attempt to deal with races between control plane and GC. GC worker sets on NFT_SET_ELEM_DEAD_BIT on elements and it creates a GC transaction to remove the expired elements, GC transaction could abort in case of interference with control plane and retried later (GC async). Set backends such as rbtree and pipapo also perform GC from control plane (GC sync), in such case, element deactivation and removal is safe because mutex is held then collected elements are released via call_rcu().

3) Adapt existing set backends to use the GC transaction API.

4) Update rhash set backend to set on _DEAD bit to report deleted elements from datapath for GC.

5) Remove old GC batch API and the NFT_SET_ELEM_BUSY_BIT.

Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

Please, pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-23-08-10

Thanks.

----------------------------------------------------------------

The following changes since commit c5ccff70501d92db445a135fa49cf9bc6b98c444:

Merge branch 'net-sched-bind-logic-fixes-for-cls_fw-cls_u32-and-cls_route' (2023-07-31 20:10:39 -0700)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-23-08-10

for you to fetch changes up to a2dd0233cbc4d8a0abb5f64487487ffc9265beb5:

netfilter: nf_tables: remove busy mark and gc batch API (2023-08-10 08:25:27 +0200)

----------------------------------------------------------------
netfilter pull request 23-08-10

----------------------------------------------------------------
Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

Quoted 6 lines
 include/net/netfilter/nf_tables.h | 120 ++++++---------
 net/netfilter/nf_tables_api.c     | 307 ++++++++++++++++++++++++++++++--------
 net/netfilter/nft_set_hash.c      |  85 +++++++----
 net/netfilter/nft_set_pipapo.c    |  66 +++++---
 net/netfilter/nft_set_rbtree.c    | 146 ++++++++++--------
 5 files changed, 476 insertions(+), 248 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

author maintainer
#9 lore raw
Quoted 80 lines · Greg KH
Quoted 72 lines · Pablo Neira Ayuso

Hi,

The following patchset contains Netfilter fixes for net.

The existing attempt to resolve races between control plane and GC work is error prone, as reported by Bien Pham <[email protected]>, some places forgot to call nft_set_elem_mark_busy(), leading to double-deactivation of elements.

This series contains the following patches:

1) Do not skip expired elements during walk otherwise elements might never decrement the reference counter on data, leading to memleak.

2) Add a GC transaction API to replace the former attempt to deal with races between control plane and GC. GC worker sets on NFT_SET_ELEM_DEAD_BIT on elements and it creates a GC transaction to remove the expired elements, GC transaction could abort in case of interference with control plane and retried later (GC async). Set backends such as rbtree and pipapo also perform GC from control plane (GC sync), in such case, element deactivation and removal is safe because mutex is held then collected elements are released via call_rcu().

3) Adapt existing set backends to use the GC transaction API.

4) Update rhash set backend to set on _DEAD bit to report deleted elements from datapath for GC.

5) Remove old GC batch API and the NFT_SET_ELEM_BUSY_BIT.

Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

Please, pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-23-08-10

Thanks.

----------------------------------------------------------------

The following changes since commit c5ccff70501d92db445a135fa49cf9bc6b98c444:

Merge branch 'net-sched-bind-logic-fixes-for-cls_fw-cls_u32-and-cls_route' (2023-07-31 20:10:39 -0700)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-23-08-10

for you to fetch changes up to a2dd0233cbc4d8a0abb5f64487487ffc9265beb5:

netfilter: nf_tables: remove busy mark and gc batch API (2023-08-10 08:25:27 +0200)

----------------------------------------------------------------
netfilter pull request 23-08-10

----------------------------------------------------------------
Florian Westphal (1):
netfilter: nf_tables: don't skip expired elements during walk

Pablo Neira Ayuso (4):
netfilter: nf_tables: GC transaction API to avoid race with control plane
netfilter: nf_tables: adapt set backend to use GC transaction API
netfilter: nft_set_hash: mark set element as dead when deleting from packet path
netfilter: nf_tables: remove busy mark and gc batch API

Quoted 6 lines
 include/net/netfilter/nf_tables.h | 120 ++++++---------
 net/netfilter/nf_tables_api.c     | 307 ++++++++++++++++++++++++++++++--------
 net/netfilter/nft_set_hash.c      |  85 +++++++----
 net/netfilter/nft_set_pipapo.c    |  66 +++++---
 net/netfilter/nft_set_rbtree.c    | 146 ++++++++++--------
 5 files changed, 476 insertions(+), 248 deletions(-)
Quoted 6 lines

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

I will re-submit this once this hit upstream.

Thanks.

maintainer
#10 lore raw
In reply to Pablo Neira Ayuso

We've got some new kdoc warnings here:

net/netfilter/nft_set_pipapo.c:1557: warning: Function parameter or member '_set' not described in 'pipapo_gc' net/netfilter/nft_set_pipapo.c:1557: warning: Excess function parameter 'set' description in 'pipapo_gc' include/net/netfilter/nf_tables.h:577: warning: Function parameter or member 'dead' not described in 'nft_set'

Don't think Linus will care enough to complain but it'd be good to get those cleaned up.