[PATCH net 0/3] Netfilter fixes for net

[PATCH net 0/3] Netfilter fixes for net

[Pablo Neira Ayuso]

Hi,

The following patchset contains Netfilter fixes for net. There is a
larger batch of fixes still pending that will follow up asap, this is
what I deemed to be more urgent at this time:

1) Use clone view in pipapo set backend to release elements from destroy
   path, otherwise it is possible to destroy elements twice.

2) Incorrect check for internal table flags lead to bogus transaction
   objects.

3) Fix counters memleak in netdev basechain update error path,
   from Quan Tian.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-24-03-21

Thanks.

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

The following changes since commit 9c6a59543a3965071d65b0f9ea43aa396ce2ed14:

  Merge branch 'octeontx2-pf-mbox-fixes' (2024-03-20 10:49:08 +0000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-24-03-21

for you to fetch changes up to 1c2e3b462542241d2e6f4d32f8356608ff51f487:

  netfilter: nf_tables: Fix a memory leak in nf_tables_updchain (2024-03-21 00:46:03 +0100)

----------------------------------------------------------------
netfilter pull request 24-03-21

----------------------------------------------------------------
Pablo Neira Ayuso (2):
      netfilter: nft_set_pipapo: release elements in clone only from destroy path
      netfilter: nf_tables: do not compare internal table flags on updates

Quan Tian (1):
      netfilter: nf_tables: Fix a memory leak in nf_tables_updchain

 net/netfilter/nf_tables_api.c  | 29 +++++++++++++++--------------
 net/netfilter/nft_set_pipapo.c |  5 +----
 2 files changed, 16 insertions(+), 18 deletions(-)

[PATCH net 1/3] netfilter: nft_set_pipapo: release elements in clone only from destroy path

[Pablo Neira Ayuso]

Clone already always provides a current view of the lookup table, use it
to destroy the set, otherwise it is possible to destroy elements twice.

This fix requires:

 212ed75dc5fb ("netfilter: nf_tables: integrate pipapo into commit protocol")

which came after:

 9827a0e6e23b ("netfilter: nft_set_pipapo: release elements in clone from abort path").

Fixes: 9827a0e6e23b ("netfilter: nft_set_pipapo: release elements in clone from abort path")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_set_pipapo.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index c0ceea068936..df8de5090246 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -2329,8 +2329,6 @@ static void nft_pipapo_destroy(const struct nft_ctx *ctx,
 	if (m) {
 		rcu_barrier();
 
-		nft_set_pipapo_match_destroy(ctx, set, m);
-
 		for_each_possible_cpu(cpu)
 			pipapo_free_scratch(m, cpu);
 		free_percpu(m->scratch);
@@ -2342,8 +2340,7 @@ static void nft_pipapo_destroy(const struct nft_ctx *ctx,
 	if (priv->clone) {
 		m = priv->clone;
 
-		if (priv->dirty)
-			nft_set_pipapo_match_destroy(ctx, set, m);
+		nft_set_pipapo_match_destroy(ctx, set, m);
 
 		for_each_possible_cpu(cpu)
 			pipapo_free_scratch(priv->clone, cpu);
-- 
2.30.2

[PATCH net 2/3] netfilter: nf_tables: do not compare internal table flags on updates

[Pablo Neira Ayuso]

Restore skipping transaction if table update does not modify flags.

Fixes: 179d9ba5559a ("netfilter: nf_tables: fix table flag updates")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e93f905e60b6..984c1c83ee38 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1213,7 +1213,7 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (flags & ~NFT_TABLE_F_MASK)
 		return -EOPNOTSUPP;
 
-	if (flags == ctx->table->flags)
+	if (flags == (ctx->table->flags & NFT_TABLE_F_MASK))
 		return 0;
 
 	if ((nft_table_has_owner(ctx->table) &&
-- 
2.30.2

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

[Pablo Neira Ayuso]

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(-)

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));
-- 
2.30.2

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

[Paolo Abeni]

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

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

[Pablo Neira Ayuso]

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.

[PATCH net 0/3,v2] Netfilter fixes for net

[Pablo Neira Ayuso]

v2: Amended missing SOB in patch 3/3.

-o-

Hi,

The following patchset contains Netfilter fixes for net. There is a
larger batch of fixes still pending that will follow up asap, this is
what I deemed to be more urgent at this time:

1) Use clone view in pipapo set backend to release elements from destroy
   path, otherwise it is possible to destroy elements twice.

2) Incorrect check for internal table flags lead to bogus transaction
   objects.

3) Fix counters memleak in netdev basechain update error path,
   from Quan Tian.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-24-03-21

Thanks.

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

The following changes since commit 9c6a59543a3965071d65b0f9ea43aa396ce2ed14:

  Merge branch 'octeontx2-pf-mbox-fixes' (2024-03-20 10:49:08 +0000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-24-03-21

for you to fetch changes up to 7eaf837a4eb5f74561e2486972e7f5184b613f6e:

  netfilter: nf_tables: Fix a memory leak in nf_tables_updchain (2024-03-21 12:12:06 +0100)

----------------------------------------------------------------
netfilter pull request 24-03-21

----------------------------------------------------------------
Pablo Neira Ayuso (2):
      netfilter: nft_set_pipapo: release elements in clone only from destroy path
      netfilter: nf_tables: do not compare internal table flags on updates

Quan Tian (1):
      netfilter: nf_tables: Fix a memory leak in nf_tables_updchain

 net/netfilter/nf_tables_api.c  | 29 +++++++++++++++--------------
 net/netfilter/nft_set_pipapo.c |  5 +----
 2 files changed, 16 insertions(+), 18 deletions(-)

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

[Pablo Neira Ayuso]

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>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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));
-- 
2.30.2