[nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[Phil Sutter]

When deleting a base chain, iptables-nft simply submits the whole chain
to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
added by fixed commit then turned this into a chain update, destroying
the hook but not the chain itself.

Detect the situation by checking if the chain's hook list becomes empty
after removing all submitted hooks from it. A base chain without hooks
is pointless, so revert back to deleting the chain.

Note the 'goto err_chain_del_hook', error path takes care of undoing the
hook_list modification and releasing the unused chain_hook.

Fixes: 7d937b107108f ("netfilter: nf_tables: support for deleting devices in an existing netdev chain")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 477c39358da7d..9666c8e891fa7 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2699,6 +2699,11 @@ static int nft_delchain_hook(struct nft_ctx *ctx, struct nft_chain *chain,
 		list_move(&hook->list, &chain_del_list);
 	}
 
+	if (list_empty(&basechain->hook_list)) {
+		err = nft_delchain(ctx);
+		goto err_chain_del_hook;
+	}
+
 	trans = nft_trans_alloc(ctx, NFT_MSG_DELCHAIN,
 				sizeof(struct nft_trans_chain));
 	if (!trans) {
-- 
2.40.0

Re: [nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi Phil,

On Fri, Jun 16, 2023 at 05:56:11PM +0200, Phil Sutter wrote:
> When deleting a base chain, iptables-nft simply submits the whole chain
> to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
> added by fixed commit then turned this into a chain update, destroying
> the hook but not the chain itself.
> 
> Detect the situation by checking if the chain's hook list becomes empty
> after removing all submitted hooks from it. A base chain without hooks
> is pointless, so revert back to deleting the chain.
> 
> Note the 'goto err_chain_del_hook', error path takes care of undoing the
> hook_list modification and releasing the unused chain_hook.

Could you give a try to this alternative patch?

Thanks.

[-- Attachment #2: fix.patch --]
[-- Type: text/x-diff, Size: 1593 bytes --]

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 99f2297f8792..1ebf3c6aba62 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2811,21 +2811,18 @@ static int nf_tables_newchain(struct sk_buff *skb, const struct nfnl_info *info,
 	return nf_tables_addchain(&ctx, family, genmask, policy, flags, extack);
 }
 
-static int nft_delchain_hook(struct nft_ctx *ctx, struct nft_chain *chain,
+static int nft_delchain_hook(struct nft_ctx *ctx,
+			     struct nft_base_chain *basechain,
 			     struct netlink_ext_ack *extack)
 {
+	const struct nft_chain *chain = &basechain->chain;
 	const struct nlattr * const *nla = ctx->nla;
 	struct nft_chain_hook chain_hook = {};
-	struct nft_base_chain *basechain;
 	struct nft_hook *this, *hook;
 	LIST_HEAD(chain_del_list);
 	struct nft_trans *trans;
 	int err;
 
-	if (!nft_is_base_chain(chain))
-		return -EOPNOTSUPP;
-
-	basechain = nft_base_chain(chain);
 	err = nft_chain_parse_hook(ctx->net, basechain, nla, &chain_hook,
 				   ctx->family, chain->flags, extack);
 	if (err < 0)
@@ -2915,7 +2912,12 @@ static int nf_tables_delchain(struct sk_buff *skb, const struct nfnl_info *info,
 		if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
 			return -EOPNOTSUPP;
 
-		return nft_delchain_hook(&ctx, chain, extack);
+		if (nft_is_base_chain(chain)) {
+			struct nft_base_chain *basechain = nft_base_chain(chain);
+
+			if (nft_base_chain_netdev(table->family, basechain->ops.hooknum))
+				return nft_delchain_hook(&ctx, basechain, extack);
+		}
 	}
 
 	if (info->nlh->nlmsg_flags & NLM_F_NONREC &&

Re: [nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Sun, Jun 18, 2023 at 04:59:38PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Jun 16, 2023 at 05:56:11PM +0200, Phil Sutter wrote:
> > When deleting a base chain, iptables-nft simply submits the whole chain
> > to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
> > added by fixed commit then turned this into a chain update, destroying
> > the hook but not the chain itself.
> > 
> > Detect the situation by checking if the chain's hook list becomes empty
> > after removing all submitted hooks from it. A base chain without hooks
> > is pointless, so revert back to deleting the chain.
> > 
> > Note the 'goto err_chain_del_hook', error path takes care of undoing the
> > hook_list modification and releasing the unused chain_hook.
> 
> Could you give a try to this alternative patch?

This is the full patch.

[-- Attachment #2: 0001-netfilter-nf_tables-Fix-for-deleting-base-chains-wit.patch --]
[-- Type: text/x-diff, Size: 2717 bytes --]

From 6d09ac39a91bffb91d6cdc08b97c03fc4f23594e Mon Sep 17 00:00:00 2001
From: Phil Sutter <phil@nwl.cc>
Date: Fri, 16 Jun 2023 17:56:11 +0200
Subject: [PATCH nf] netfilter: nf_tables: Fix for deleting base chains with
 payload

When deleting a base chain, iptables-nft simply submits the whole chain
to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
added by fixed commit then turned this into a chain update, destroying
the hook but not the chain itself.

Detect the situation by checking if the chain's hook list becomes empty
after removing all submitted hooks from it. A base chain without hooks
is pointless, so revert back to deleting the chain.

Note the 'goto err_chain_del_hook', error path takes care of undoing the
hook_list modification and releasing the unused chain_hook.

Fixes: 7d937b107108f ("netfilter: nf_tables: support for deleting devices in an existing netdev chain")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c1db2f4b2aa4..4c7937fd803f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2811,21 +2811,18 @@ static int nf_tables_newchain(struct sk_buff *skb, const struct nfnl_info *info,
 	return nf_tables_addchain(&ctx, family, genmask, policy, flags, extack);
 }
 
-static int nft_delchain_hook(struct nft_ctx *ctx, struct nft_chain *chain,
+static int nft_delchain_hook(struct nft_ctx *ctx,
+			     struct nft_base_chain *basechain,
 			     struct netlink_ext_ack *extack)
 {
+	const struct nft_chain *chain = &basechain->chain;
 	const struct nlattr * const *nla = ctx->nla;
 	struct nft_chain_hook chain_hook = {};
-	struct nft_base_chain *basechain;
 	struct nft_hook *this, *hook;
 	LIST_HEAD(chain_del_list);
 	struct nft_trans *trans;
 	int err;
 
-	if (!nft_is_base_chain(chain))
-		return -EOPNOTSUPP;
-
-	basechain = nft_base_chain(chain);
 	err = nft_chain_parse_hook(ctx->net, basechain, nla, &chain_hook,
 				   ctx->family, chain->flags, extack);
 	if (err < 0)
@@ -2910,7 +2907,12 @@ static int nf_tables_delchain(struct sk_buff *skb, const struct nfnl_info *info,
 		if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
 			return -EOPNOTSUPP;
 
-		return nft_delchain_hook(&ctx, chain, extack);
+		if (nft_is_base_chain(chain)) {
+			struct nft_base_chain *basechain = nft_base_chain(chain);
+
+			if (nft_base_chain_netdev(table->family, basechain->ops.hooknum))
+				return nft_delchain_hook(&ctx, basechain, extack);
+		}
 	}
 
 	if (info->nlh->nlmsg_flags & NLM_F_NONREC &&
-- 
2.30.2

Re: [nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Mon, Jun 19, 2023 at 10:55:59AM +0200, Pablo Neira Ayuso wrote:
> On Sun, Jun 18, 2023 at 04:59:38PM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Fri, Jun 16, 2023 at 05:56:11PM +0200, Phil Sutter wrote:
> > > When deleting a base chain, iptables-nft simply submits the whole chain
> > > to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
> > > added by fixed commit then turned this into a chain update, destroying
> > > the hook but not the chain itself.
> > > 
> > > Detect the situation by checking if the chain's hook list becomes empty
> > > after removing all submitted hooks from it. A base chain without hooks
> > > is pointless, so revert back to deleting the chain.
> > > 
> > > Note the 'goto err_chain_del_hook', error path takes care of undoing the
> > > hook_list modification and releasing the unused chain_hook.
> > 
> > Could you give a try to this alternative patch?
> 
> This is the full patch.

I forgot to mangle the patch description describing the new approach.

[-- Attachment #2: 0001-netfilter-nf_tables-Fix-for-deleting-base-chains-wit.patch --]
[-- Type: text/x-diff, Size: 2473 bytes --]

From 3da13f15a02e065e12080f2d66f81289aa6ebd69 Mon Sep 17 00:00:00 2001
From: Phil Sutter <phil@nwl.cc>
Date: Fri, 16 Jun 2023 17:56:11 +0200
Subject: [PATCH] netfilter: nf_tables: Fix for deleting base chains with
 payload

When deleting a base chain, iptables-nft simply submits the whole chain
to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
added by fixed commit then turned this into a chain update, destroying
the hook but not the chain itself. Detect the situation by checking if
the chain is either the netdev or inet/ingress type.

Fixes: 7d937b107108f ("netfilter: nf_tables: support for deleting devices in an existing netdev chain")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c1db2f4b2aa4..4c7937fd803f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2811,21 +2811,18 @@ static int nf_tables_newchain(struct sk_buff *skb, const struct nfnl_info *info,
 	return nf_tables_addchain(&ctx, family, genmask, policy, flags, extack);
 }
 
-static int nft_delchain_hook(struct nft_ctx *ctx, struct nft_chain *chain,
+static int nft_delchain_hook(struct nft_ctx *ctx,
+			     struct nft_base_chain *basechain,
 			     struct netlink_ext_ack *extack)
 {
+	const struct nft_chain *chain = &basechain->chain;
 	const struct nlattr * const *nla = ctx->nla;
 	struct nft_chain_hook chain_hook = {};
-	struct nft_base_chain *basechain;
 	struct nft_hook *this, *hook;
 	LIST_HEAD(chain_del_list);
 	struct nft_trans *trans;
 	int err;
 
-	if (!nft_is_base_chain(chain))
-		return -EOPNOTSUPP;
-
-	basechain = nft_base_chain(chain);
 	err = nft_chain_parse_hook(ctx->net, basechain, nla, &chain_hook,
 				   ctx->family, chain->flags, extack);
 	if (err < 0)
@@ -2910,7 +2907,12 @@ static int nf_tables_delchain(struct sk_buff *skb, const struct nfnl_info *info,
 		if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
 			return -EOPNOTSUPP;
 
-		return nft_delchain_hook(&ctx, chain, extack);
+		if (nft_is_base_chain(chain)) {
+			struct nft_base_chain *basechain = nft_base_chain(chain);
+
+			if (nft_base_chain_netdev(table->family, basechain->ops.hooknum))
+				return nft_delchain_hook(&ctx, basechain, extack);
+		}
 	}
 
 	if (info->nlh->nlmsg_flags & NLM_F_NONREC &&
-- 
2.30.2

Re: [nf PATCH] netfilter: nf_tables: Fix for deleting base chains with payload

[Phil Sutter]

On Mon, Jun 19, 2023 at 10:58:52AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jun 19, 2023 at 10:55:59AM +0200, Pablo Neira Ayuso wrote:
> > On Sun, Jun 18, 2023 at 04:59:38PM +0200, Pablo Neira Ayuso wrote:
> > > Hi Phil,
> > > 
> > > On Fri, Jun 16, 2023 at 05:56:11PM +0200, Phil Sutter wrote:
> > > > When deleting a base chain, iptables-nft simply submits the whole chain
> > > > to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
> > > > added by fixed commit then turned this into a chain update, destroying
> > > > the hook but not the chain itself.
> > > > 
> > > > Detect the situation by checking if the chain's hook list becomes empty
> > > > after removing all submitted hooks from it. A base chain without hooks
> > > > is pointless, so revert back to deleting the chain.
> > > > 
> > > > Note the 'goto err_chain_del_hook', error path takes care of undoing the
> > > > hook_list modification and releasing the unused chain_hook.
> > > 
> > > Could you give a try to this alternative patch?

Ah, I guess my fix breaks temporary removal of a single interface from a
netdev chain?

> > This is the full patch.

Works fine, iptables testsuite is back to normal again. :)

> I forgot to mangle the patch description describing the new approach.

> From 3da13f15a02e065e12080f2d66f81289aa6ebd69 Mon Sep 17 00:00:00 2001
> From: Phil Sutter <phil@nwl.cc>
> Date: Fri, 16 Jun 2023 17:56:11 +0200
> Subject: [PATCH] netfilter: nf_tables: Fix for deleting base chains with
>  payload
> 
> When deleting a base chain, iptables-nft simply submits the whole chain
> to the kernel, including the NFTA_CHAIN_HOOK attribute. The new code
> added by fixed commit then turned this into a chain update, destroying
> the hook but not the chain itself. Detect the situation by checking if
> the chain is either the netdev or inet/ingress type.

I'd change the last sentence to "Avoid the situation by applying the new
logic to base chains in netdev family or inet family ingress hook." But
feel free to push as-is, I'm just nit-picking here. :)

Thanks, Phil