netfilter: Reinstate dropped Stable patches
Subsystems (3)
cover netfilter: Reinstate dropped Stable patches
Dropped in August:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/commit/?id=da9e96b4de57f6621f21e682bad92b5ffed0eeee
https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/commit/?id=8d1dbdfdd7612a9ea73b005f75a3b2b8b0610d4e
Only re-applied and build tested against v6.1.53.
If they do apply and test well against linux-5.{4,10,15}, all the better.
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(-)
Signature · 1 line
2.42.0.459.ge4e396fd5e-goog
PATCH 1/5 netfilter: nf_tables: don't skip expired elements during walk
From: Florian Westphal <[email protected]>
[ Upstream commit 24138933b97b055d486e8064b4a1721702442a9b ]
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.
--- 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
@@ -5386,8 +5386,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); }
net/netfilter/nft_set_hash.c +0 -2
@@ -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
@@ -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; }/**
@@ -2024,8 +2032,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
@@ -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.42.0.459.ge4e396fd5e-goog
PATCH 2/5 netfilter: nf_tables: GC transaction API to avoid race with control plane
In reply to Lee JonesFrom: Pablo Neira Ayuso <[email protected]>
[ Upstream commit 5f68718b34a531a556f2f50300ead2862278da26 ]
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.
--- 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
@@ -507,6 +507,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
@@ -536,6 +537,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;
@@ -557,7 +559,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;
@@ -1577,6 +1580,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 *
@@ -1708,6 +1737,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 );
@@ -1735,6 +1796,7 @@ struct nftables_pernet {u64 table_handle;unsigned int base_seq;u8 validate_state; +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.42.0.459.ge4e396fd5e-goog
PATCH 3/5 netfilter: nf_tables: adapt set backend to use GC transaction API
In reply to Lee JonesFrom: Pablo Neira Ayuso <[email protected]>
[ Upstream commit f6c383b8c31a93752a52697f8430a71dcbc46adf ]
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.
--- 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
@@ -6153,7 +6153,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); }
@@ -6168,8 +6167,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);
@@ -6880,8 +6878,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.42.0.459.ge4e396fd5e-goog
PATCH 4/5 netfilter: nft_set_hash: mark set element as dead when deleting from packet path
In reply to Lee JonesFrom: Pablo Neira Ayuso <[email protected]>
[ Upstream commit c92db3030492b8ad1d0faace7a93bbcf53850d0c ]
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.
--- net/netfilter/nft_set_hash.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
net/netfilter/nft_set_hash.c +4 -2
@@ -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.42.0.459.ge4e396fd5e-goog
PATCH 5/5 netfilter: nf_tables: remove busy mark and gc batch API
In reply to Lee JonesFrom: Pablo Neira Ayuso <[email protected]>
[ Upstream commit a2dd0233cbc4d8a0abb5f64487487ffc9265beb5 ]
Ditch it, it has been replace it by the GC transaction API and it has no clients anymore.
--- 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
@@ -594,7 +594,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) {
@@ -811,62 +810,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
@@ -1545,47 +1488,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.42.0.459.ge4e396fd5e-goog
Pablo Neira Ayuso
maintainerHi,
Quoted 8 lines · Lee Jones
Dropped in August:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/commit/?id=da9e96b4de57f6621f21e682bad92b5ffed0eeee
https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/commit/?id=8d1dbdfdd7612a9ea73b005f75a3b2b8b0610d4eOnly re-applied and build tested against v6.1.53.
If they do apply and test well against linux-5.{4,10,15}, all the better.
I have a pending queue of patches for all these trees.
I will follow up asap, please hold on with this.
Pablo Neira Ayuso
maintainerQuoted 8 lines · Pablo Neira Ayuso
Hi,
Quoted 7 lines
Quoted 3 lines
Only re-applied and build tested against v6.1.53.
If they do apply and test well against linux-5.{4,10,15}, all the better.
I have a pending queue of patches for all these trees.
I will follow up asap, please hold on with this.
For the record, this is my pending list of -stable patches:
https://people.netfilter.org/pablo/linux-stable/
I will rebase on top of current -stable releases and post them. There is also a few more recent fixes not yet in that queue that are also good to have.
so, please, let me take care of this.
Thanks.
Quoted 19 lines · Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso
Hi,
Quoted 7 lines
Quoted 3 lines
Only re-applied and build tested against v6.1.53.
If they do apply and test well against linux-5.{4,10,15}, all the better.
I have a pending queue of patches for all these trees.
I will follow up asap, please hold on with this.
For the record, this is my pending list of -stable patches:
Quoted 7 lines
https://people.netfilter.org/pablo/linux-stable/
I will rebase on top of current -stable releases and post them. There is also a few more recent fixes not yet in that queue that are also good to have.
so, please, let me take care of this.
Sure.
Signature · 1 line
Lee Jones [李琼斯]