diff mbox series

[net,3/3] netfilter: nf_tables: Fix a memory leak in nf_tables_updchain

Message ID 20240321000635.31865-4-pablo@netfilter.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,1/3] netfilter: nft_set_pipapo: release elements in clone only from destroy path | expand

Checks

Context Check Description
netdev/series_format success Pull request is its own cover letter
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 955 this patch: 955
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 3 maintainers not CCed: coreteam@netfilter.org fw@strlen.de kadlec@netfilter.org
netdev/build_clang success Errors and warnings before: 956 this patch: 956
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 972 this patch: 972
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 39 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-03-21--06-00 (tests: 908)

Commit Message

Pablo Neira Ayuso March 21, 2024, 12:06 a.m. UTC 
From: Quan Tian <tianquan23@gmail.com>

If nft_netdev_register_hooks() fails, the memory associated with
nft_stats is not freed, causing a memory leak.

This patch fixes it by moving nft_stats_alloc() down after
nft_netdev_register_hooks() succeeds.

Fixes: b9703ed44ffb ("netfilter: nf_tables: support for adding new devices to an existing netdev chain")
Signed-off-by: Quan Tian <tianquan23@gmail.com>
---
 net/netfilter/nf_tables_api.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Comments

Paolo Abeni March 21, 2024, 10:52 a.m. UTC | #1 
On Thu, 2024-03-21 at 01:06 +0100, Pablo Neira Ayuso wrote:
> From: Quan Tian <tianquan23@gmail.com>
> 
> If nft_netdev_register_hooks() fails, the memory associated with
> nft_stats is not freed, causing a memory leak.
> 
> This patch fixes it by moving nft_stats_alloc() down after
> nft_netdev_register_hooks() succeeds.
> 
> Fixes: b9703ed44ffb ("netfilter: nf_tables: support for adding new devices to an existing netdev chain")
> Signed-off-by: Quan Tian <tianquan23@gmail.com>

I'm sorry for nit-picking, but our tag verification scripts are unhappy
WRT this commit, it lacks your SoB. Would you mind sending an updated
PR?

Thanks!

Paolo
Pablo Neira Ayuso March 21, 2024, 11:11 a.m. UTC | #2 
On Thu, Mar 21, 2024 at 11:52:29AM +0100, Paolo Abeni wrote:
> On Thu, 2024-03-21 at 01:06 +0100, Pablo Neira Ayuso wrote:
> > From: Quan Tian <tianquan23@gmail.com>
> > 
> > If nft_netdev_register_hooks() fails, the memory associated with
> > nft_stats is not freed, causing a memory leak.
> > 
> > This patch fixes it by moving nft_stats_alloc() down after
> > nft_netdev_register_hooks() succeeds.
> > 
> > Fixes: b9703ed44ffb ("netfilter: nf_tables: support for adding new devices to an existing netdev chain")
> > Signed-off-by: Quan Tian <tianquan23@gmail.com>
> 
> I'm sorry for nit-picking, but our tag verification scripts are unhappy
> WRT this commit, it lacks your SoB. Would you mind sending an updated
> PR?

Sure, sorry about this.
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 984c1c83ee38..5fa3d3540c93 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2631,19 +2631,6 @@  static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 		}
 	}
 
-	if (nla[NFTA_CHAIN_COUNTERS]) {
-		if (!nft_is_base_chain(chain)) {
-			err = -EOPNOTSUPP;
-			goto err_hooks;
-		}
-
-		stats = nft_stats_alloc(nla[NFTA_CHAIN_COUNTERS]);
-		if (IS_ERR(stats)) {
-			err = PTR_ERR(stats);
-			goto err_hooks;
-		}
-	}
-
 	if (!(table->flags & NFT_TABLE_F_DORMANT) &&
 	    nft_is_base_chain(chain) &&
 	    !list_empty(&hook.list)) {
@@ -2658,6 +2645,20 @@  static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 	}
 
 	unregister = true;
+
+	if (nla[NFTA_CHAIN_COUNTERS]) {
+		if (!nft_is_base_chain(chain)) {
+			err = -EOPNOTSUPP;
+			goto err_hooks;
+		}
+
+		stats = nft_stats_alloc(nla[NFTA_CHAIN_COUNTERS]);
+		if (IS_ERR(stats)) {
+			err = PTR_ERR(stats);
+			goto err_hooks;
+		}
+	}
+
 	err = -ENOMEM;
 	trans = nft_trans_alloc(ctx, NFT_MSG_NEWCHAIN,
 				sizeof(struct nft_trans_chain));