| Message ID | 20230919133616.20436-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [nf] netfilter: nf_tables: fix memory leak when more than 255 elements expired | expand |
Florian Westphal <fw@strlen.de> wrote: > When more than 255 elements expire we're supposed to switch to a new gc > container structure, but nft_trans_gc_space() always returns false in this Grrr. This should read 'always returns true' or 'never returns false', but not *THIS*. I'll fix this up when applying this, probably tomorrow morning.
On Tue, Sep 19, 2023 at 03:59:38PM +0200, Florian Westphal wrote: > Florian Westphal <fw@strlen.de> wrote: > > When more than 255 elements expire we're supposed to switch to a new gc > > container structure, but nft_trans_gc_space() always returns false in this > > Grrr. This should read 'always returns true' or 'never returns false', > but not *THIS*. I'll fix this up when applying this, probably tomorrow > morning. You could amend the fix when applying this.
On Tue, Sep 19, 2023 at 04:12:34PM +0200, Pablo Neira Ayuso wrote: > On Tue, Sep 19, 2023 at 03:59:38PM +0200, Florian Westphal wrote: > > Florian Westphal <fw@strlen.de> wrote: > > > When more than 255 elements expire we're supposed to switch to a new gc > > > container structure, but nft_trans_gc_space() always returns false in this > > > > Grrr. This should read 'always returns true' or 'never returns false', > > but not *THIS*. I'll fix this up when applying this, probably tomorrow > > morning. > > You could amend the fix when applying this. I mean, this patch description, as you prefer.
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index dd40c75011d2..a338d7d4f69b 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -1682,7 +1682,7 @@ struct nft_trans_gc { struct net *net; struct nft_set *set; u32 seq; - u8 count; + u16 count; void *priv[NFT_TRANS_GC_BATCHCOUNT]; struct rcu_head rcu; }; diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index e429ebba74b3..96d40d62dd5a 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -9562,12 +9562,15 @@ static int nft_trans_gc_space(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) { + struct nft_set *set; + if (nft_trans_gc_space(gc)) return gc; + set = gc->set; nft_trans_gc_queue_work(gc); - return nft_trans_gc_alloc(gc->set, gc_seq, gfp); + return nft_trans_gc_alloc(set, gc_seq, gfp); } void nft_trans_gc_queue_async_done(struct nft_trans_gc *trans) @@ -9582,15 +9585,18 @@ void nft_trans_gc_queue_async_done(struct nft_trans_gc *trans) struct nft_trans_gc *nft_trans_gc_queue_sync(struct nft_trans_gc *gc, gfp_t gfp) { + struct nft_set *set; + if (WARN_ON_ONCE(!lockdep_commit_lock_is_held(gc->net))) return NULL; if (nft_trans_gc_space(gc)) return gc; + set = gc->set; call_rcu(&gc->rcu, nft_trans_gc_trans_free); - return nft_trans_gc_alloc(gc->set, 0, gfp); + return nft_trans_gc_alloc(set, 0, gfp); } void nft_trans_gc_queue_sync_done(struct nft_trans_gc *trans)
When more than 255 elements expire we're supposed to switch to a new gc container structure, but nft_trans_gc_space() always returns false in this case because u8 type is too narrow for the boundary. While at it, don't deref 'gc' after we've passed it to call_rcu. Fixes: 5f68718b34a5 ("netfilter: nf_tables: GC transaction API to avoid race with control plane") Reported-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- include/net/netfilter/nf_tables.h | 2 +- net/netfilter/nf_tables_api.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)