linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
@ 2023-09-11 21:37 Timo Sigurdsson
  2023-09-11 22:57 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Timo Sigurdsson @ 2023-09-11 21:37 UTC (permalink / raw)
  To: pablo, kadlec, fw, davem, edumazet, kuba, pabeni,
	netfilter-devel, coreteam, netdev, linux-kernel, stable,
	regressions, sashal
  Cc: carnil, 1051592

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

Hi,

recently, Debian updated their stable kernel from 6.1.38 to 6.1.52 which broke nftables ruleset loading on one of my machines with lots of "Operation not supported" errors. I've reported this to the Debian project (see link below) and Salvatore Bonaccorso and I identified "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit that introduced the regression. Salvatore also found that this issue affects the 5.10 stable tree as well (observed in 5.10.191), but he cannot reproduce it on 6.4.13 and 6.5.2.

The issue only occurs with some rulesets. While I can't trigger it with simple/minimal rulesets that I use on some machines, it does occur with a more complex ruleset that has been in use for months (if not years, for large parts of it). I'm attaching a somewhat stripped down version of the ruleset from the machine I originally observed this issue on. It's still not a small or simple ruleset, but I'll try to reduce it further when I have more time.

The error messages shown when trying to load the ruleset don't seem to be helpful. Just two simple examples:
Just to give two simple examples from the log when nftables fails to start:
/etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not supported
                        tcp option maxseg size 1-500 counter drop
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not supported
                        tcp dport sip-tls accept
                        ^^^^^^^^^^^^^^^^^^^^^^^^

Since the issue only affects some stable trees, Salvatore thought it might be an incomplete backport that causes this.

If you need further information, please let me know.


Thanks and kind regards,

Timo


#regzbot introduced: 0ebc1064e487
#regzbot monitor: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1051592

[-- Attachment #2: nftables.conf --]
[-- Type: text/plain, Size: 9259 bytes --]

#!/usr/sbin/nft -f

flush ruleset

define public_if = eth0
define trusted_if = eth1
define voip_if = eth2.10
define guest_if = eth2.20
define home_if = { $trusted_if, $voip_if, $guest_if }
define home_ipv6_if = { $trusted_if, $voip_if, $guest_if }

define masq_ip = { 192.168.1.0/24, 192.168.2.0/24, 192.168.3.0/24, 192.168.4.0/24 }
define masq_if = $public_if

define host1_ip = 192.168.1.10
define host2_ip = 192.168.2.20
define host3_ip = 192.168.3.30
define host4_ip = 192.168.4.40

define proxy_port = 8443

define private_ip = { 192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8 }
define private_ip6 = { fe80::/64, fd00::/8 }
define bogons_ip = { 0.0.0.0/8, 10.0.0.0/8, 100.64.0.0/10, 127.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.0.0.0/24, 192.0.2.0/24, 192.168.0.0/16, 198.18.0.0/15, 198.51.100.0/24, 203.0.113.0/24, 224.0.0.0/3 }
define bogons_ip6 = { ::/3, 2001:0002::/48, 2001:0003::/32, 2001:10::/28, 2001:20::/28, 2001::/32, 2001:db8::/32, 2002::/16, 3000::/4, 4000::/2, 8000::/1 }

define sip_whitelist_ip6 = { 2001:db8::1/128, 2001:db8::2/128 }
define smtps_whitelist_ip = 10.0.0.1
define protocol_whitelist = { tcp, udp, icmp, ipv6-icmp }

table inet filter {
	map if_input {
		type ifname : verdict;
		elements = { $public_if : jump public_input, $trusted_if : jump home_input, $voip_if : jump home_input, $guest_if : jump home_input }
	}
	map if_forward {
		type ifname : verdict;
		elements = { $public_if : jump public_forward, $trusted_if : jump trusted_forward, $voip_if : jump voip_forward, $guest_if : jump guest_forward }
	}
	map if_output {
		type ifname : verdict;
		elements = { $public_if : jump public_output, $trusted_if : jump home_output, $voip_if : jump home_output, $guest_if : jump home_output }
	}

	set ipv4_blacklist { type ipv4_addr; flags interval; auto-merge; }
	set ipv6_blacklist { type ipv6_addr; flags interval; auto-merge; }
	set limit_src_ip { type ipv4_addr; flags dynamic, timeout; size 1024; }
	set limit_src_ip6 { type ipv6_addr; flags dynamic, timeout; size 1024; }

	chain PREROUTING_RAW {
		type filter hook prerouting priority raw;

		meta l4proto != $protocol_whitelist counter drop
		tcp flags syn jump {
			tcp option maxseg size 1-500 counter drop
			tcp sport 0 counter drop
		}
		rt type 0 counter drop
	}

	chain PREROUTING_MANGLE {
		type filter hook prerouting priority mangle;

		ct state vmap { invalid : jump ct_invalid_pre, untracked : jump ct_untracked_pre, new : jump ct_new_pre, related : jump rpfilter }
	}
	chain ct_invalid_pre {
		counter drop
	}
	chain ct_untracked_pre {
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert, mld-listener-query, mld2-listener-report } return
		counter drop
	}
	chain ct_new_pre {
		jump rpfilter

		tcp flags & (fin|syn|rst|ack) != syn counter drop

		iifname $public_if meta nfproto vmap { ipv4 : jump blacklist_input_ipv4, ipv6 : jump blacklist_input_ipv6 }
	}
	chain rpfilter {
		ip saddr 0.0.0.0 ip daddr 255.255.255.255 udp sport bootpc udp dport bootps return
		ip6 saddr ::/128 ip6 daddr . icmpv6 type { ff02::1:ff00:0/104 . nd-neighbor-solicit, ff02::16 . mld2-listener-report } return

		fib saddr . iif oif eq 0 counter drop
	}
	chain blacklist_input_ipv4{
		ip saddr $bogons_ip counter drop
		ip saddr @ipv4_blacklist counter drop
	}
	chain blacklist_input_ipv6{
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } ip6 saddr fe80::/64 return
		udp sport dhcpv6-server ip6 saddr fe80::/64 return

		ip6 saddr $bogons_ip6 counter drop
		ip6 saddr @ipv6_blacklist counter drop
	}

	chain INPUT {
		type filter hook input priority filter; policy drop;

		iif lo accept

		ct state established,related accept

		iifname vmap @if_input

		log prefix "NFT REJECT IN " flags ether flags ip options limit rate 5/second burst 10 packets reject
	}
	chain public_input {
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } ip6 saddr fe80::/64 ip6 hoplimit 255 accept

		udp sport dhcpv6-server udp dport dhcpv6-client ip6 saddr fe80::/64 accept
		fib daddr type { broadcast, multicast, anycast } counter drop

		counter drop
	}
	chain home_input {
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } ip6 hoplimit 255 accept
		icmpv6 type { mld-listener-query, mld2-listener-report } ip6 hoplimit 1 accept

		udp sport bootpc udp dport bootps accept
		udp sport dhcpv6-client udp dport dhcpv6-server iifname $home_ipv6_if accept

		fib daddr type { broadcast, multicast, anycast } counter drop

		icmp type echo-request accept
		icmpv6 type echo-request accept

		tcp dport ssh iifname $trusted_if accept

		meta l4proto { tcp, udp } th dport domain jump {
			ip6 saddr != $private_ip6 counter reject
			accept
		}

		udp dport ntp accept

		tcp dport $proxy_port accept
	}

	chain FORWARD_MANGLE {
		type filter hook forward priority mangle;

		oifname $public_if jump {
			ct state new meta nfproto vmap { ipv4 : jump blacklist_output_ipv4, ipv6 : jump blacklist_output_ipv6 }
			tcp flags & (syn|rst) == syn tcp option maxseg size set rt mtu
		}
	}
	chain blacklist_output_ipv4 {
		ip daddr $bogons_ip goto log_blacklist
		ip daddr @ipv4_blacklist goto log_blacklist
	}
	chain blacklist_output_ipv6 {
		icmpv6 type . ip6 daddr { nd-router-solicit . ff02::2/128, nd-neighbor-solicit . ff02::1:ff00:0/104, nd-neighbor-advert . fe80::/64, nd-neighbor-advert . ff02::1/128, nd-neighbor-advert . ff02::1:ff00:0/104, mld2-listener-report . ff02::16/128 } return
		udp dport dhcpv6-server ip6 daddr ff02::1:2 return

		ip6 daddr $bogons_ip6 goto log_blacklist
		ip6 daddr @ipv6_blacklist goto log_blacklist
	}
	chain log_blacklist {
		log prefix "NFT BLACKLIST " flags ether flags ip options limit rate 5/minute burst 10 packets drop
		counter drop
	}

	chain FORWARD {
		type filter hook forward priority filter; policy drop;

		ct state established,related accept

		fib daddr type { broadcast, multicast, anycast } counter drop

		iifname vmap @if_forward

		log prefix "NFT REJECT FWD " flags ether flags ip options limit rate 5/second burst 10 packets reject
	}
	chain public_forward {
		udp dport { sip, 7078-7097 } oifname $voip_if jump {
			ip6 saddr $sip_whitelist_ip6 accept
			meta nfproto ipv6 log prefix "NFT DROP SIP " flags ether flags ip options limit rate 5/second burst 10 packets drop
		}

		counter drop
	}
	chain trusted_forward {
		oifname $public_if accept

		icmp type echo-request accept
		icmpv6 type echo-request accept

		ip daddr { $host3_ip, $host4_ip } tcp dport vmap { ssh : accept, https : accept, http : drop }

		ip daddr $host2_ip jump {
			tcp dport { http, https, printer, ipp, 9100 } accept
			udp dport snmp accept
		}
	}
	chain voip_forward {
		icmpv6 type { destination-unreachable, packet-too-big, time-exceeded, parameter-problem, echo-request } oifname $public_if accept

		ip6 daddr $sip_whitelist_ip6 jump {
			udp dport { 3478, sip } accept
			udp sport { 7078-7097 } accept
			tcp dport sip-tls accept
		}

		tcp dport submissions ip daddr $smtps_whitelist_ip accept
		tcp dport http oifname $public_if counter reject
	}
	chain guest_forward {
		oifname $public_if accept
	}

	chain OUTPUT {
		type filter hook output priority filter; policy drop;

		oif lo accept

		ct state vmap { established : accept, related : accept, invalid : jump ct_invalid_out, untracked : jump ct_untracked_out }

		oifname vmap @if_output

		log prefix "NFT REJECT OUT " flags ether flags ip options limit rate 5/second burst 10 packets reject
	}
	chain ct_invalid_out {
		counter drop
	}
	chain ct_untracked_out {
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert, mld-listener-query, mld2-listener-report } return
		counter drop
	}
	chain public_output {
		ct state new meta nfproto vmap { ipv4 : jump blacklist_output_ipv4, ipv6 : jump blacklist_output_ipv6 }

		icmp type { destination-unreachable, time-exceeded, parameter-problem, echo-request } accept
		icmpv6 type { destination-unreachable, packet-too-big, time-exceeded, parameter-problem, echo-request } accept
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } ip6 hoplimit 255 accept
		icmpv6 type { mld-listener-query, mld2-listener-report } ip6 hoplimit 1 accept

		udp dport dhcpv6-server ip6 saddr fe80::/64 ip6 daddr ff02::1:2 accept

		udp dport { domain, ntp } accept
		tcp dport { https, submissions, domain-s } accept
	}
	chain home_output {
		icmp type { destination-unreachable, time-exceeded, parameter-problem, echo-request } accept
		icmpv6 type { destination-unreachable, packet-too-big, time-exceeded, parameter-problem, echo-request } accept
		icmpv6 type { nd-router-solicit, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } ip6 hoplimit 255 accept
		icmpv6 type { mld-listener-query, mld2-listener-report } ip6 hoplimit 1 accept

		udp sport dhcpv6-server udp dport dhcpv6-client ip6 saddr fe80::/64 oifname $home_ipv6_if accept
		udp sport bootps udp dport bootpc ip saddr $private_ip accept
		tcp dport ssh ip daddr $host1_ip accept
	}

	chain POSTROUTING_SRCNAT {
		type nat hook postrouting priority srcnat;

		meta nfproto ipv4 ip saddr $masq_ip oifname $masq_if masquerade
	}
}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-11 21:37 Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable Timo Sigurdsson
@ 2023-09-11 22:57 ` Pablo Neira Ayuso
  2023-09-12  8:32   ` Linux regression tracking (Thorsten Leemhuis)
  2023-09-12 11:39   ` Timo Sigurdsson
  0 siblings, 2 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2023-09-11 22:57 UTC (permalink / raw)
  To: Timo Sigurdsson
  Cc: kadlec, fw, davem, edumazet, kuba, pabeni, netfilter-devel,
	coreteam, netdev, linux-kernel, stable, regressions, sashal,
	carnil, 1051592

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

Hi Timo,

On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
> Hi,
> 
> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
> which broke nftables ruleset loading on one of my machines with lots
> of "Operation not supported" errors. I've reported this to the
> Debian project (see link below) and Salvatore Bonaccorso and I
> identified "netfilter: nf_tables: disallow rule addition to bound
> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
> that introduced the regression. Salvatore also found that this issue
> affects the 5.10 stable tree as well (observed in 5.10.191), but he
> cannot reproduce it on 6.4.13 and 6.5.2.
> 
> The issue only occurs with some rulesets. While I can't trigger it
> with simple/minimal rulesets that I use on some machines, it does
> occur with a more complex ruleset that has been in use for months
> (if not years, for large parts of it). I'm attaching a somewhat
> stripped down version of the ruleset from the machine I originally
> observed this issue on. It's still not a small or simple ruleset,
> but I'll try to reduce it further when I have more time.
> 
> The error messages shown when trying to load the ruleset don't seem
> to be helpful. Just two simple examples: Just to give two simple
> examples from the log when nftables fails to start:
> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not supported
>                         tcp option maxseg size 1-500 counter drop
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not supported
>                         tcp dport sip-tls accept
>                         ^^^^^^^^^^^^^^^^^^^^^^^^

I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
this is not reproducible with v1.0.7 and v1.0.8.

> Since the issue only affects some stable trees, Salvatore thought it
> might be an incomplete backport that causes this.
> 
> If you need further information, please let me know.

Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
kernel check that rejects adding rules to bound chains. The incorrect
bytecode adds the chain binding, attach it to the rule and it adds the
rules to the chain binding. I have cherry-picked these three patches
for nftables v1.0.6 userspace and your ruleset restores fine.

See patches enclosed to this email.

[-- Attachment #2: 0001-rule-add-helper-function-to-expand-chain-rules-into-.patch --]
[-- Type: text/x-diff, Size: 2424 bytes --]

From 4e5b0a64227dde250f94bec45b3fb127d78b7fd2 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 6 Feb 2023 15:28:40 +0100
Subject: [PATCH 1/3,nft] rule: add helper function to expand chain rules intoi
 commands

[ upstream commit 784597a4ed63b9decb10d74fdb49a1b021e22728 ]

This patch adds a helper function to expand chain rules into commands.
This comes in preparation for the follow up patch.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/rule.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 1402210acd8d..43c6520517ce 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1310,13 +1310,31 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
 	cmd->num_attrs++;
 }
 
+static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds)
+{
+	struct rule *rule;
+	struct handle h;
+	struct cmd *new;
+
+	list_for_each_entry(rule, &chain->rules, list) {
+		memset(&h, 0, sizeof(h));
+		handle_merge(&h, &rule->handle);
+		if (chain->flags & CHAIN_F_BINDING) {
+			rule->handle.chain_id = chain->handle.chain_id;
+			rule->handle.chain.location = chain->location;
+		}
+		new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
+				&rule->location, rule_get(rule));
+		list_add_tail(&new->list, new_cmds);
+	}
+}
+
 void nft_cmd_expand(struct cmd *cmd)
 {
 	struct list_head new_cmds;
 	struct flowtable *ft;
 	struct table *table;
 	struct chain *chain;
-	struct rule *rule;
 	struct set *set;
 	struct obj *obj;
 	struct cmd *new;
@@ -1362,22 +1380,9 @@ void nft_cmd_expand(struct cmd *cmd)
 					&ft->location, flowtable_get(ft));
 			list_add_tail(&new->list, &new_cmds);
 		}
-		list_for_each_entry(chain, &table->chains, list) {
-			list_for_each_entry(rule, &chain->rules, list) {
-				memset(&h, 0, sizeof(h));
-				handle_merge(&h, &rule->handle);
-				if (chain->flags & CHAIN_F_BINDING) {
-					rule->handle.chain_id =
-						chain->handle.chain_id;
-					rule->handle.chain.location =
-						chain->location;
-				}
-				new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
-						&rule->location,
-						rule_get(rule));
-				list_add_tail(&new->list, &new_cmds);
-			}
-		}
+		list_for_each_entry(chain, &table->chains, list)
+			nft_cmd_expand_chain(chain, &new_cmds);
+
 		list_splice(&new_cmds, &cmd->list);
 		break;
 	case CMD_OBJ_SET:
-- 
2.30.2


[-- Attachment #3: 0002-rule-expand-standalone-chain-that-contains-rules.patch --]
[-- Type: text/x-diff, Size: 3636 bytes --]

From 70c03d81df0e87fb416bd1f38409367e9d08ed7f Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 6 Feb 2023 15:28:41 +0100
Subject: [PATCH 2/3,nft] rule: expand standalone chain that contains rules

[ upstream 27c753e4a8d4744f479345e3f5e34cafef751602 commit ]

Otherwise rules that this chain contains are ignored when expressed
using the following syntax:

chain inet filter input2 {
       type filter hook input priority filter; policy accept;
       ip saddr 1.2.3.4 tcp dport { 22, 443, 123 } drop
}

When expanding the chain, remove the rule so the new CMD_OBJ_CHAIN
case does not expand it again.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1655
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/rule.c                                    | 15 +++++++++---
 .../testcases/include/0020include_chain_0     | 23 +++++++++++++++++++
 .../include/dumps/0020include_chain_0.nft     |  6 +++++
 3 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100755 tests/shell/testcases/include/0020include_chain_0
 create mode 100644 tests/shell/testcases/include/dumps/0020include_chain_0.nft

diff --git a/src/rule.c b/src/rule.c
index 43c6520517ce..323d89afd141 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1312,11 +1312,12 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, const struct location *loc)
 
 static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds)
 {
-	struct rule *rule;
+	struct rule *rule, *next;
 	struct handle h;
 	struct cmd *new;
 
-	list_for_each_entry(rule, &chain->rules, list) {
+	list_for_each_entry_safe(rule, next, &chain->rules, list) {
+		list_del(&rule->list);
 		memset(&h, 0, sizeof(h));
 		handle_merge(&h, &rule->handle);
 		if (chain->flags & CHAIN_F_BINDING) {
@@ -1324,7 +1325,7 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
 			rule->handle.chain.location = chain->location;
 		}
 		new = cmd_alloc(CMD_ADD, CMD_OBJ_RULE, &h,
-				&rule->location, rule_get(rule));
+				&rule->location, rule);
 		list_add_tail(&new->list, new_cmds);
 	}
 }
@@ -1385,6 +1386,14 @@ void nft_cmd_expand(struct cmd *cmd)
 
 		list_splice(&new_cmds, &cmd->list);
 		break;
+	case CMD_OBJ_CHAIN:
+		chain = cmd->chain;
+		if (!chain || list_empty(&chain->rules))
+			break;
+
+		nft_cmd_expand_chain(chain, &new_cmds);
+		list_splice(&new_cmds, &cmd->list);
+		break;
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 		set = cmd->set;
diff --git a/tests/shell/testcases/include/0020include_chain_0 b/tests/shell/testcases/include/0020include_chain_0
new file mode 100755
index 000000000000..8f78e8c606ec
--- /dev/null
+++ b/tests/shell/testcases/include/0020include_chain_0
@@ -0,0 +1,23 @@
+#!/bin/bash
+
+set -e
+
+tmpfile1=$(mktemp -p .)
+if [ ! -w $tmpfile1 ] ; then
+	echo "Failed to create tmp file" >&2
+	exit 0
+fi
+
+trap "rm -rf $tmpfile1" EXIT # cleanup if aborted
+
+RULESET="table inet filter { }
+include \"$tmpfile1\""
+
+RULESET2="chain inet filter input2 {
+	type filter hook input priority filter; policy accept;
+	ip saddr 1.2.3.4 tcp dport { 22, 443, 123 } drop
+}"
+
+echo "$RULESET2" > $tmpfile1
+
+$NFT -o -f - <<< $RULESET
diff --git a/tests/shell/testcases/include/dumps/0020include_chain_0.nft b/tests/shell/testcases/include/dumps/0020include_chain_0.nft
new file mode 100644
index 000000000000..3ad6db14d2f5
--- /dev/null
+++ b/tests/shell/testcases/include/dumps/0020include_chain_0.nft
@@ -0,0 +1,6 @@
+table inet filter {
+	chain input2 {
+		type filter hook input priority filter; policy accept;
+		ip saddr 1.2.3.4 tcp dport { 22, 123, 443 } drop
+	}
+}
-- 
2.30.2


[-- Attachment #4: 0003-src-expand-table-command-before-evaluation.patch --]
[-- Type: text/x-diff, Size: 6509 bytes --]

From a13ffae3838548d9b337d6efe74af070edc355d5 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 23 Feb 2023 19:55:39 +0100
Subject: [PATCH 3/3,nft] src: expand table command before evaluation

[ upstream 3975430b12d97c92cdf03753342f2269153d5624 commit ]

The nested syntax notation results in one single table command which
includes all other objects. This differs from the flat notation where
there is usually one command per object.

This patch adds a previous step to the evaluation phase to expand the
objects that are contained in the table into independent commands, so
both notations have similar representations.

Remove the code to evaluate the nested representation in the evaluation
phase since commands are independently evaluated after the expansion.

The commands are expanded after the set element collapse step, in case
that there is a long list of singleton element commands to be added to
the set, to shorten the command list iteration.

This approach also avoids interference with the object cache that is
populated in the evaluation, which might refer to objects coming in the
existing command list that is being processed.

There is still a post_expand phase to detach the elements from the set
which could be consolidated by updating the evaluation step to handle
the CMD_OBJ_SETELEMS command type.

This patch fixes 27c753e4a8d4 ("rule: expand standalone chain that
contains rules") which broke rule addition/insertion by index because
the expansion code after the evaluation messes up the cache.

Fixes: 27c753e4a8d4 ("rule: expand standalone chain that contains rules")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h    |  1 +
 src/evaluate.c    | 39 ---------------------------------------
 src/libnftables.c |  9 ++++++++-
 src/rule.c        | 19 ++++++++++++++++++-
 4 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index 00a1bac5a773..7625addfd919 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -733,6 +733,7 @@ extern struct cmd *cmd_alloc(enum cmd_ops op, enum cmd_obj obj,
 			     const struct handle *h, const struct location *loc,
 			     void *data);
 extern void nft_cmd_expand(struct cmd *cmd);
+extern void nft_cmd_post_expand(struct cmd *cmd);
 extern bool nft_cmd_collapse(struct list_head *cmds);
 extern void nft_cmd_uncollapse(struct list_head *cmds);
 extern struct cmd *cmd_alloc_obj_ct(enum cmd_ops op, int type,
diff --git a/src/evaluate.c b/src/evaluate.c
index c04cb91d3919..e2172569cdcc 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4601,7 +4601,6 @@ static uint32_t str2hooknum(uint32_t family, const char *hook)
 static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 {
 	struct table *table;
-	struct rule *rule;
 
 	table = table_cache_find(&ctx->nft->cache.table_cache,
 				 ctx->cmd->handle.table.name,
@@ -4657,11 +4656,6 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 		}
 	}
 
-	list_for_each_entry(rule, &chain->rules, list) {
-		handle_merge(&rule->handle, &chain->handle);
-		if (rule_evaluate(ctx, rule, CMD_INVALID) < 0)
-			return -1;
-	}
 	return 0;
 }
 
@@ -4729,11 +4723,6 @@ static int obj_evaluate(struct eval_ctx *ctx, struct obj *obj)
 
 static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 {
-	struct flowtable *ft;
-	struct chain *chain;
-	struct set *set;
-	struct obj *obj;
-
 	if (!table_cache_find(&ctx->nft->cache.table_cache,
 			      ctx->cmd->handle.table.name,
 			      ctx->cmd->handle.family)) {
@@ -4746,34 +4735,6 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 		}
 	}
 
-	if (ctx->cmd->table == NULL)
-		return 0;
-
-	ctx->table = table;
-	list_for_each_entry(set, &table->sets, list) {
-		expr_set_context(&ctx->ectx, NULL, 0);
-		handle_merge(&set->handle, &table->handle);
-		if (set_evaluate(ctx, set) < 0)
-			return -1;
-	}
-	list_for_each_entry(chain, &table->chains, list) {
-		handle_merge(&chain->handle, &table->handle);
-		ctx->cmd->handle.chain.location = chain->location;
-		if (chain_evaluate(ctx, chain) < 0)
-			return -1;
-	}
-	list_for_each_entry(ft, &table->flowtables, list) {
-		handle_merge(&ft->handle, &table->handle);
-		if (flowtable_evaluate(ctx, ft) < 0)
-			return -1;
-	}
-	list_for_each_entry(obj, &table->objs, list) {
-		handle_merge(&obj->handle, &table->handle);
-		if (obj_evaluate(ctx, obj) < 0)
-			return -1;
-	}
-
-	ctx->table = NULL;
 	return 0;
 }
 
diff --git a/src/libnftables.c b/src/libnftables.c
index a376825d7309..fd4ee6988c27 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -520,6 +520,13 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 	if (nft_cmd_collapse(cmds))
 		collapsed = true;
 
+	list_for_each_entry(cmd, cmds, list) {
+		if (cmd->op != CMD_ADD)
+			continue;
+
+		nft_cmd_expand(cmd);
+	}
+
 	list_for_each_entry_safe(cmd, next, cmds, list) {
 		struct eval_ctx ectx = {
 			.nft	= nft,
@@ -543,7 +550,7 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 		if (cmd->op != CMD_ADD)
 			continue;
 
-		nft_cmd_expand(cmd);
+		nft_cmd_post_expand(cmd);
 	}
 
 	return 0;
diff --git a/src/rule.c b/src/rule.c
index 323d89afd141..982160359aea 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1318,8 +1318,9 @@ static void nft_cmd_expand_chain(struct chain *chain, struct list_head *new_cmds
 
 	list_for_each_entry_safe(rule, next, &chain->rules, list) {
 		list_del(&rule->list);
+		handle_merge(&rule->handle, &chain->handle);
 		memset(&h, 0, sizeof(h));
-		handle_merge(&h, &rule->handle);
+		handle_merge(&h, &chain->handle);
 		if (chain->flags & CHAIN_F_BINDING) {
 			rule->handle.chain_id = chain->handle.chain_id;
 			rule->handle.chain.location = chain->location;
@@ -1350,6 +1351,7 @@ void nft_cmd_expand(struct cmd *cmd)
 			return;
 
 		list_for_each_entry(chain, &table->chains, list) {
+			handle_merge(&chain->handle, &table->handle);
 			memset(&h, 0, sizeof(h));
 			handle_merge(&h, &chain->handle);
 			h.chain_id = chain->handle.chain_id;
@@ -1394,6 +1396,21 @@ void nft_cmd_expand(struct cmd *cmd)
 		nft_cmd_expand_chain(chain, &new_cmds);
 		list_splice(&new_cmds, &cmd->list);
 		break;
+	default:
+		break;
+	}
+}
+
+void nft_cmd_post_expand(struct cmd *cmd)
+{
+	struct list_head new_cmds;
+	struct set *set;
+	struct cmd *new;
+	struct handle h;
+
+	init_list_head(&new_cmds);
+
+	switch (cmd->obj) {
 	case CMD_OBJ_SET:
 	case CMD_OBJ_MAP:
 		set = cmd->set;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-11 22:57 ` Pablo Neira Ayuso
@ 2023-09-12  8:32   ` Linux regression tracking (Thorsten Leemhuis)
  2023-09-12 10:27     ` Florian Westphal
  2023-09-12 11:39   ` Timo Sigurdsson
  1 sibling, 1 reply; 11+ messages in thread
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-09-12  8:32 UTC (permalink / raw)
  To: Pablo Neira Ayuso, Timo Sigurdsson
  Cc: kadlec, fw, davem, edumazet, kuba, pabeni, netfilter-devel,
	coreteam, netdev, linux-kernel, stable, regressions, sashal,
	carnil, 1051592

On 12.09.23 00:57, Pablo Neira Ayuso wrote:
> On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
>>
>> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
>> which broke nftables ruleset loading on one of my machines with lots
>> of "Operation not supported" errors. I've reported this to the
>> Debian project (see link below) and Salvatore Bonaccorso and I
>> identified "netfilter: nf_tables: disallow rule addition to bound
>> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
>> that introduced the regression. Salvatore also found that this issue
>> affects the 5.10 stable tree as well (observed in 5.10.191), but he
>> cannot reproduce it on 6.4.13 and 6.5.2.
>>
>> The issue only occurs with some rulesets. While I can't trigger it
>> with simple/minimal rulesets that I use on some machines, it does
>> occur with a more complex ruleset that has been in use for months
>> (if not years, for large parts of it). I'm attaching a somewhat
>> stripped down version of the ruleset from the machine I originally
>> observed this issue on. It's still not a small or simple ruleset,
>> but I'll try to reduce it further when I have more time.
>>
>> The error messages shown when trying to load the ruleset don't seem
>> to be helpful. Just two simple examples: Just to give two simple
>> examples from the log when nftables fails to start:
>> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not supported
>>                         tcp option maxseg size 1-500 counter drop
>>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not supported
>>                         tcp dport sip-tls accept
>>                         ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
> this is not reproducible with v1.0.7 and v1.0.8.
> 
>> Since the issue only affects some stable trees, Salvatore thought it
>> might be an incomplete backport that causes this.
>>
>> If you need further information, please let me know.
> 
> Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> kernel check that rejects adding rules to bound chains. The incorrect
> bytecode adds the chain binding, attach it to the rule and it adds the
> rules to the chain binding. I have cherry-picked these three patches
> for nftables v1.0.6 userspace and your ruleset restores fine.
> [...]

Hmmmm. Well, this sounds like a kernel regression to me that normally
should be dealt with on the kernel level, as users after updating the
kernel should never have to update any userspace stuff to continue what
they have been doing before the kernel update.

Can't the kernel somehow detect the incorrect bytecode and do the right
thing(tm) somehow?

But yes, don't worry, I know that reality is not black and white and
that it's crucial that things like package filtering do exactly what the
user expect it to do; that's why this might be one of those rare
situations where "user has to update userspace components to support
newer kernels" might be the better of two bad choices. But I had to ask
to ensure it's something like that.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12  8:32   ` Linux regression tracking (Thorsten Leemhuis)
@ 2023-09-12 10:27     ` Florian Westphal
  2023-09-12 11:47       ` Timo Sigurdsson
  2023-09-29 11:46       ` Linux regression tracking (Thorsten Leemhuis)
  0 siblings, 2 replies; 11+ messages in thread
From: Florian Westphal @ 2023-09-12 10:27 UTC (permalink / raw)
  To: Linux regressions mailing list
  Cc: Pablo Neira Ayuso, Timo Sigurdsson, kadlec, fw, davem, edumazet,
	kuba, pabeni, netfilter-devel, coreteam, netdev, linux-kernel,
	stable, sashal, carnil, 1051592

Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info> wrote:
> On 12.09.23 00:57, Pablo Neira Ayuso wrote:
> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> > kernel check that rejects adding rules to bound chains. The incorrect
> > bytecode adds the chain binding, attach it to the rule and it adds the
> > rules to the chain binding. I have cherry-picked these three patches
> > for nftables v1.0.6 userspace and your ruleset restores fine.
> > [...]
> 
> Hmmmm. Well, this sounds like a kernel regression to me that normally
> should be dealt with on the kernel level, as users after updating the
> kernel should never have to update any userspace stuff to continue what
> they have been doing before the kernel update.

This is a combo of a userspace bug and this new sanity check that
rejects the incorrect ordering (adding rules to the already-bound
anonymous chain).

nf_tables uses a transaction allor-nothing model, this means that any
error that occurs during a transaction has to be reverse/undo all the
pending changes.  This has caused a myriad of bugs already.

So while this can be theoretically fixed in the kernel I don't see
a sane way to do it.  Error unwinding / recovery from deeply nested
errors is already too complex for my taste.

> Can't the kernel somehow detect the incorrect bytecode and do the right
> thing(tm) somehow?

Theoretically yes, but I don't feel competent enough to do it, just look
at all the UaF bugs of the past month.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-11 22:57 ` Pablo Neira Ayuso
  2023-09-12  8:32   ` Linux regression tracking (Thorsten Leemhuis)
@ 2023-09-12 11:39   ` Timo Sigurdsson
  2023-09-12 11:58     ` Pablo Neira Ayuso
  2023-09-12 19:13     ` Salvatore Bonaccorso
  1 sibling, 2 replies; 11+ messages in thread
From: Timo Sigurdsson @ 2023-09-12 11:39 UTC (permalink / raw)
  To: pablo
  Cc: kadlec, fw, davem, edumazet, kuba, pabeni, netfilter-devel,
	coreteam, netdev, linux-kernel, stable, regressions, sashal,
	carnil, 1051592

Hi Pablo,

Pablo Neira Ayuso schrieb am 12.09.2023 00:57 (GMT +02:00):

> Hi Timo,
> 
> On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
>> Hi,
>> 
>> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
>> which broke nftables ruleset loading on one of my machines with lots
>> of "Operation not supported" errors. I've reported this to the
>> Debian project (see link below) and Salvatore Bonaccorso and I
>> identified "netfilter: nf_tables: disallow rule addition to bound
>> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
>> that introduced the regression. Salvatore also found that this issue
>> affects the 5.10 stable tree as well (observed in 5.10.191), but he
>> cannot reproduce it on 6.4.13 and 6.5.2.
>> 
>> The issue only occurs with some rulesets. While I can't trigger it
>> with simple/minimal rulesets that I use on some machines, it does
>> occur with a more complex ruleset that has been in use for months
>> (if not years, for large parts of it). I'm attaching a somewhat
>> stripped down version of the ruleset from the machine I originally
>> observed this issue on. It's still not a small or simple ruleset,
>> but I'll try to reduce it further when I have more time.
>> 
>> The error messages shown when trying to load the ruleset don't seem
>> to be helpful. Just two simple examples: Just to give two simple
>> examples from the log when nftables fails to start:
>> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not
>> supported
>>                         tcp option maxseg size 1-500 counter drop
>>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not
>> supported
>>                         tcp dport sip-tls accept
>>                         ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
> this is not reproducible with v1.0.7 and v1.0.8.
> 
>> Since the issue only affects some stable trees, Salvatore thought it
>> might be an incomplete backport that causes this.
>> 
>> If you need further information, please let me know.
> 
> Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> kernel check that rejects adding rules to bound chains. The incorrect
> bytecode adds the chain binding, attach it to the rule and it adds the
> rules to the chain binding. I have cherry-picked these three patches
> for nftables v1.0.6 userspace and your ruleset restores fine.

hmm, that doesn't explain why Salvatore didn't observe this with more recent kernels.

Salvatore, did you use newer userspace components when you tested your 6.4.13 and 6.5.2 builds?

As for the regression and how it be dealt with: Personally, I don't really care whether the regression is solved in the kernel or userspace. If everybody agrees that this is the best or only viable option and Debian decides to push a nftables update to fix this, that works for me. But I do feel the burden to justify this should be high. A kernel change that leaves users without a working packet filter after upgrading their machines is serious, if you ask me. And since it affects several stable/longterm trees, I would assume this will hit other stable (non-rolling) distributions as well, since they will also use older userspace components (unless this is behavior specific to nftables 1.0.6 but not older versions). They probably should get a heads up then.


Regards,

Timo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 10:27     ` Florian Westphal
@ 2023-09-12 11:47       ` Timo Sigurdsson
  2023-09-12 11:52         ` Florian Westphal
  2023-09-29 11:46       ` Linux regression tracking (Thorsten Leemhuis)
  1 sibling, 1 reply; 11+ messages in thread
From: Timo Sigurdsson @ 2023-09-12 11:47 UTC (permalink / raw)
  To: regressions, fw
  Cc: pablo, kadlec, fw, davem, edumazet, kuba, pabeni,
	netfilter-devel, coreteam, netdev, linux-kernel, stable, sashal,
	carnil, 1051592

Hi,

Florian Westphal schrieb am 12.09.2023 12:27 (GMT +02:00):

> Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
> wrote:
>> On 12.09.23 00:57, Pablo Neira Ayuso wrote:
>> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
>> > kernel check that rejects adding rules to bound chains. The incorrect
>> > bytecode adds the chain binding, attach it to the rule and it adds the
>> > rules to the chain binding. I have cherry-picked these three patches
>> > for nftables v1.0.6 userspace and your ruleset restores fine.
>> > [...]
>> 
>> Hmmmm. Well, this sounds like a kernel regression to me that normally
>> should be dealt with on the kernel level, as users after updating the
>> kernel should never have to update any userspace stuff to continue what
>> they have been doing before the kernel update.
> 
> This is a combo of a userspace bug and this new sanity check that
> rejects the incorrect ordering (adding rules to the already-bound
> anonymous chain).
> 

Out of curiosity, did the incorrect ordering or bytecode from the older userspace components actually lead to a wrong representation of the rules in the kernel or did the rules still work despite all that?

Thanks,

Timo 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 11:47       ` Timo Sigurdsson
@ 2023-09-12 11:52         ` Florian Westphal
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2023-09-12 11:52 UTC (permalink / raw)
  To: Timo Sigurdsson
  Cc: regressions, fw, pablo, kadlec, davem, edumazet, kuba, pabeni,
	netfilter-devel, coreteam, netdev, linux-kernel, stable, sashal,
	carnil, 1051592

Timo Sigurdsson <public_timo.s@silentcreek.de> wrote:
> > Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info>
> > wrote:
> >> On 12.09.23 00:57, Pablo Neira Ayuso wrote:
> >> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> >> > kernel check that rejects adding rules to bound chains. The incorrect
> >> > bytecode adds the chain binding, attach it to the rule and it adds the
> >> > rules to the chain binding. I have cherry-picked these three patches
> >> > for nftables v1.0.6 userspace and your ruleset restores fine.
> >> > [...]
> >> 
> >> Hmmmm. Well, this sounds like a kernel regression to me that normally
> >> should be dealt with on the kernel level, as users after updating the
> >> kernel should never have to update any userspace stuff to continue what
> >> they have been doing before the kernel update.
> > 
> > This is a combo of a userspace bug and this new sanity check that
> > rejects the incorrect ordering (adding rules to the already-bound
> > anonymous chain).
> > 
> 
> Out of curiosity, did the incorrect ordering or bytecode from the older userspace components actually lead to a wrong representation of the rules in the kernel or did the rules still work despite all that?

It works, but without the stricter behaviour userspace can trigger
memory corruption in the kernel. nftables userland will not trigger this.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 11:39   ` Timo Sigurdsson
@ 2023-09-12 11:58     ` Pablo Neira Ayuso
  2023-09-12 19:13     ` Salvatore Bonaccorso
  1 sibling, 0 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2023-09-12 11:58 UTC (permalink / raw)
  To: Timo Sigurdsson
  Cc: kadlec, fw, davem, edumazet, kuba, pabeni, netfilter-devel,
	coreteam, netdev, linux-kernel, stable, regressions, sashal,
	carnil, 1051592

On Tue, Sep 12, 2023 at 01:39:59PM +0200, Timo Sigurdsson wrote:
> Hi Pablo,
> 
> Pablo Neira Ayuso schrieb am 12.09.2023 00:57 (GMT +02:00):
> 
> > Hi Timo,
> > 
> > On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
> >> Hi,
> >> 
> >> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
> >> which broke nftables ruleset loading on one of my machines with lots
> >> of "Operation not supported" errors. I've reported this to the
> >> Debian project (see link below) and Salvatore Bonaccorso and I
> >> identified "netfilter: nf_tables: disallow rule addition to bound
> >> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
> >> that introduced the regression. Salvatore also found that this issue
> >> affects the 5.10 stable tree as well (observed in 5.10.191), but he
> >> cannot reproduce it on 6.4.13 and 6.5.2.
> >> 
> >> The issue only occurs with some rulesets. While I can't trigger it
> >> with simple/minimal rulesets that I use on some machines, it does
> >> occur with a more complex ruleset that has been in use for months
> >> (if not years, for large parts of it). I'm attaching a somewhat
> >> stripped down version of the ruleset from the machine I originally
> >> observed this issue on. It's still not a small or simple ruleset,
> >> but I'll try to reduce it further when I have more time.
> >> 
> >> The error messages shown when trying to load the ruleset don't seem
> >> to be helpful. Just two simple examples: Just to give two simple
> >> examples from the log when nftables fails to start:
> >> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not
> >> supported
> >>                         tcp option maxseg size 1-500 counter drop
> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not
> >> supported
> >>                         tcp dport sip-tls accept
> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
> > this is not reproducible with v1.0.7 and v1.0.8.
> > 
> >> Since the issue only affects some stable trees, Salvatore thought it
> >> might be an incomplete backport that causes this.
> >> 
> >> If you need further information, please let me know.
> > 
> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> > kernel check that rejects adding rules to bound chains. The incorrect
> > bytecode adds the chain binding, attach it to the rule and it adds the
> > rules to the chain binding. I have cherry-picked these three patches
> > for nftables v1.0.6 userspace and your ruleset restores fine.
> 
> hmm, that doesn't explain why Salvatore didn't observe this with
> more recent kernels.
>
> Salvatore, did you use newer userspace components when you tested
> your 6.4.13 and 6.5.2 builds?
> 
> As for the regression and how it be dealt with: Personally, I don't
> really care whether the regression is solved in the kernel or
> userspace. If everybody agrees that this is the best or only viable
> option and Debian decides to push a nftables update to fix this,
> that works for me. But I do feel the burden to justify this should
> be high. A kernel change that leaves users without a working packet
> filter after upgrading their machines is serious, if you ask me. And
> since it affects several stable/longterm trees, I would assume this
> will hit other stable (non-rolling) distributions as well, since
> they will also use older userspace components (unless this is
> behavior specific to nftables 1.0.6 but not older versions). They
> probably should get a heads up then.

There is coverage for the chain binding feature in our tests
infrastructure, but unfortunately the bug did not trigger with newer
nftables versions. In the future, I will keep an eye with running our
tests on older userspace nftables versions when working on stable
trees.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 11:39   ` Timo Sigurdsson
  2023-09-12 11:58     ` Pablo Neira Ayuso
@ 2023-09-12 19:13     ` Salvatore Bonaccorso
  2023-09-15 20:44       ` Timo Sigurdsson
  1 sibling, 1 reply; 11+ messages in thread
From: Salvatore Bonaccorso @ 2023-09-12 19:13 UTC (permalink / raw)
  To: Timo Sigurdsson
  Cc: pablo, kadlec, fw, davem, edumazet, kuba, pabeni,
	netfilter-devel, coreteam, netdev, linux-kernel, stable,
	regressions, sashal, 1051592, Arturo Borrero Gonzalez

Hi Timo,

On Tue, Sep 12, 2023 at 01:39:59PM +0200, Timo Sigurdsson wrote:
> Hi Pablo,
> 
> Pablo Neira Ayuso schrieb am 12.09.2023 00:57 (GMT +02:00):
> 
> > Hi Timo,
> > 
> > On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
> >> Hi,
> >> 
> >> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
> >> which broke nftables ruleset loading on one of my machines with lots
> >> of "Operation not supported" errors. I've reported this to the
> >> Debian project (see link below) and Salvatore Bonaccorso and I
> >> identified "netfilter: nf_tables: disallow rule addition to bound
> >> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
> >> that introduced the regression. Salvatore also found that this issue
> >> affects the 5.10 stable tree as well (observed in 5.10.191), but he
> >> cannot reproduce it on 6.4.13 and 6.5.2.
> >> 
> >> The issue only occurs with some rulesets. While I can't trigger it
> >> with simple/minimal rulesets that I use on some machines, it does
> >> occur with a more complex ruleset that has been in use for months
> >> (if not years, for large parts of it). I'm attaching a somewhat
> >> stripped down version of the ruleset from the machine I originally
> >> observed this issue on. It's still not a small or simple ruleset,
> >> but I'll try to reduce it further when I have more time.
> >> 
> >> The error messages shown when trying to load the ruleset don't seem
> >> to be helpful. Just two simple examples: Just to give two simple
> >> examples from the log when nftables fails to start:
> >> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not
> >> supported
> >>                         tcp option maxseg size 1-500 counter drop
> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not
> >> supported
> >>                         tcp dport sip-tls accept
> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
> > this is not reproducible with v1.0.7 and v1.0.8.
> > 
> >> Since the issue only affects some stable trees, Salvatore thought it
> >> might be an incomplete backport that causes this.
> >> 
> >> If you need further information, please let me know.
> > 
> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
> > kernel check that rejects adding rules to bound chains. The incorrect
> > bytecode adds the chain binding, attach it to the rule and it adds the
> > rules to the chain binding. I have cherry-picked these three patches
> > for nftables v1.0.6 userspace and your ruleset restores fine.
> 
> hmm, that doesn't explain why Salvatore didn't observe this with
> more recent kernels.
> 
> Salvatore, did you use newer userspace components when you tested
> your 6.4.13 and 6.5.2 builds?

It does explain now because understanding the issue better. While one
while experinting should only change each one constraint for the
6.4.13 and 6.5.2 testing I indeed switched to a Debian unstable
system, which has newer userpace nftables and so not triggering the
issue. This was missleading for the report.

> As for the regression and how it be dealt with: Personally, I don't
> really care whether the regression is solved in the kernel or
> userspace. If everybody agrees that this is the best or only viable
> option and Debian decides to push a nftables update to fix this,
> that works for me. But I do feel the burden to justify this should
> be high. A kernel change that leaves users without a working packet
> filter after upgrading their machines is serious, if you ask me. And
> since it affects several stable/longterm trees, I would assume this
> will hit other stable (non-rolling) distributions as well, since
> they will also use older userspace components (unless this is
> behavior specific to nftables 1.0.6 but not older versions). They
> probably should get a heads up then.

So if it is generally believed on kernel side there should not happen
any further changes to work with older userland, I guess in Debian we
will need to patch nftables. I'm CC'ing Arturo Borrero Gonzalez
<arturo@debian.org>, maintainer for the package. The update should go
ideally in the next point releases from October (and maybe released
earlier as well trough the stable-updates mechanism).

FWIW: In Debian bullseye we have 0.9.8 based nftables, in bookworm
1.0.6, so both will need those fixes.

As 0ebc1064e487 is to address CVE-2023-4147 other distros picking the
fix will likely encounter the problem at some point. It looks Red Hat
has taken it (some RHSA's were released), I assume Ubuntu will shortly
as well release USN's containing a fix.

Regards,
Salvatore

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 19:13     ` Salvatore Bonaccorso
@ 2023-09-15 20:44       ` Timo Sigurdsson
  0 siblings, 0 replies; 11+ messages in thread
From: Timo Sigurdsson @ 2023-09-15 20:44 UTC (permalink / raw)
  To: carnil
  Cc: pablo, kadlec, fw, davem, edumazet, kuba, pabeni,
	netfilter-devel, coreteam, netdev, linux-kernel, stable,
	regressions, sashal, 1051592, arturo

Hi,

Salvatore Bonaccorso schrieb am 12.09.2023 21:13 (GMT +02:00):

> Hi Timo,
> 
> On Tue, Sep 12, 2023 at 01:39:59PM +0200, Timo Sigurdsson wrote:
>> Hi Pablo,
>> 
>> Pablo Neira Ayuso schrieb am 12.09.2023 00:57 (GMT +02:00):
>> 
>> > Hi Timo,
>> > 
>> > On Mon, Sep 11, 2023 at 11:37:50PM +0200, Timo Sigurdsson wrote:
>> >> Hi,
>> >> 
>> >> recently, Debian updated their stable kernel from 6.1.38 to 6.1.52
>> >> which broke nftables ruleset loading on one of my machines with lots
>> >> of "Operation not supported" errors. I've reported this to the
>> >> Debian project (see link below) and Salvatore Bonaccorso and I
>> >> identified "netfilter: nf_tables: disallow rule addition to bound
>> >> chain via NFTA_RULE_CHAIN_ID" (0ebc1064e487) as the offending commit
>> >> that introduced the regression. Salvatore also found that this issue
>> >> affects the 5.10 stable tree as well (observed in 5.10.191), but he
>> >> cannot reproduce it on 6.4.13 and 6.5.2.
>> >> 
>> >> The issue only occurs with some rulesets. While I can't trigger it
>> >> with simple/minimal rulesets that I use on some machines, it does
>> >> occur with a more complex ruleset that has been in use for months
>> >> (if not years, for large parts of it). I'm attaching a somewhat
>> >> stripped down version of the ruleset from the machine I originally
>> >> observed this issue on. It's still not a small or simple ruleset,
>> >> but I'll try to reduce it further when I have more time.
>> >> 
>> >> The error messages shown when trying to load the ruleset don't seem
>> >> to be helpful. Just two simple examples: Just to give two simple
>> >> examples from the log when nftables fails to start:
>> >> /etc/nftables.conf:99:4-44: Error: Could not process rule: Operation not
>> >> supported
>> >>                         tcp option maxseg size 1-500 counter drop
>> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >> /etc/nftables.conf:308:4-27: Error: Could not process rule: Operation not
>> >> supported
>> >>                         tcp dport sip-tls accept
>> >>                         ^^^^^^^^^^^^^^^^^^^^^^^^
>> > 
>> > I can reproduce this issue with 5.10.191 and 6.1.52 and nftables v1.0.6,
>> > this is not reproducible with v1.0.7 and v1.0.8.
>> > 
>> >> Since the issue only affects some stable trees, Salvatore thought it
>> >> might be an incomplete backport that causes this.
>> >> 
>> >> If you need further information, please let me know.
>> > 
>> > Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
>> > kernel check that rejects adding rules to bound chains. The incorrect
>> > bytecode adds the chain binding, attach it to the rule and it adds the
>> > rules to the chain binding. I have cherry-picked these three patches
>> > for nftables v1.0.6 userspace and your ruleset restores fine.
>> 
>> hmm, that doesn't explain why Salvatore didn't observe this with
>> more recent kernels.
>> 
>> Salvatore, did you use newer userspace components when you tested
>> your 6.4.13 and 6.5.2 builds?
> 
> It does explain now because understanding the issue better. While one
> while experinting should only change each one constraint for the
> 6.4.13 and 6.5.2 testing I indeed switched to a Debian unstable
> system, which has newer userpace nftables and so not triggering the
> issue. This was missleading for the report.
> 
>> As for the regression and how it be dealt with: Personally, I don't
>> really care whether the regression is solved in the kernel or
>> userspace. If everybody agrees that this is the best or only viable
>> option and Debian decides to push a nftables update to fix this,
>> that works for me. But I do feel the burden to justify this should
>> be high. A kernel change that leaves users without a working packet
>> filter after upgrading their machines is serious, if you ask me. And
>> since it affects several stable/longterm trees, I would assume this
>> will hit other stable (non-rolling) distributions as well, since
>> they will also use older userspace components (unless this is
>> behavior specific to nftables 1.0.6 but not older versions). They
>> probably should get a heads up then.
> 
> So if it is generally believed on kernel side there should not happen
> any further changes to work with older userland, I guess in Debian we
> will need to patch nftables. I'm CC'ing Arturo Borrero Gonzalez
> <arturo@debian.org>, maintainer for the package. The update should go
> ideally in the next point releases from October (and maybe released
> earlier as well trough the stable-updates mechanism).

So, I built nftables 1.0.6-2+deb12u1 with the three cherry-picked patches from Pablo and can confirm that they resolve the issue for me on bookworm. I can now run linux 6.1.52-1 and load my original nftables ruleset again.
 
> FWIW: In Debian bullseye we have 0.9.8 based nftables, in bookworm
> 1.0.6, so both will need those fixes.
> 
> As 0ebc1064e487 is to address CVE-2023-4147 other distros picking the
> fix will likely encounter the problem at some point. It looks Red Hat
> has taken it (some RHSA's were released), I assume Ubuntu will shortly
> as well release USN's containing a fix.

SUSE has also picked this patch for SLES/SLED. I hope maintainers follow the mailing lists cc'ed here or that someone gives them a heads up before this hits more production systems.

Thanks and regards,

Timo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable
  2023-09-12 10:27     ` Florian Westphal
  2023-09-12 11:47       ` Timo Sigurdsson
@ 2023-09-29 11:46       ` Linux regression tracking (Thorsten Leemhuis)
  1 sibling, 0 replies; 11+ messages in thread
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-09-29 11:46 UTC (permalink / raw)
  To: Florian Westphal, Linux regressions mailing list
  Cc: Pablo Neira Ayuso, Timo Sigurdsson, kadlec, davem, edumazet,
	kuba, pabeni, netfilter-devel, coreteam, netdev, linux-kernel,
	stable, sashal, carnil, 1051592

On 12.09.23 12:27, Florian Westphal wrote:
> Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info> wrote:
>> On 12.09.23 00:57, Pablo Neira Ayuso wrote:
>>> Userspace nftables v1.0.6 generates incorrect bytecode that hits a new
>>> kernel check that rejects adding rules to bound chains. The incorrect
>>> bytecode adds the chain binding, attach it to the rule and it adds the
>>> rules to the chain binding. I have cherry-picked these three patches
>>> for nftables v1.0.6 userspace and your ruleset restores fine.
>>> [...]
>>
>> Hmmmm. Well, this sounds like a kernel regression to me that normally
>> should be dealt with on the kernel level, as users after updating the
>> kernel should never have to update any userspace stuff to continue what
>> they have been doing before the kernel update.
> 
> This is a combo of a userspace bug and this new sanity check that
> rejects the incorrect ordering (adding rules to the already-bound
> anonymous chain).
> 
> nf_tables uses a transaction allor-nothing model, this means that any
> error that occurs during a transaction has to be reverse/undo all the
> pending changes.  This has caused a myriad of bugs already.
> 
> So while this can be theoretically fixed in the kernel I don't see
> a sane way to do it.  Error unwinding / recovery from deeply nested
> errors is already too complex for my taste.
> 
>> Can't the kernel somehow detect the incorrect bytecode and do the right
>> thing(tm) somehow?
> 
> Theoretically yes, but I don't feel competent enough to do it, just look
> at all the UaF bugs of the past month.

Thx for the answer. FWIW, as this was a judgement call I mentioned this
in my last regression report to Linus; he didn't reply, so I guess it is
-- and will remove this issue from my tracking:

#regzbot resolve: can be solved by a nftables userspace update; not
nice, but likely best solution in this case
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-09-29 11:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-11 21:37 Regression: Commit "netfilter: nf_tables: disallow rule addition to bound chain via NFTA_RULE_CHAIN_ID" breaks ruleset loading in linux-stable Timo Sigurdsson
2023-09-11 22:57 ` Pablo Neira Ayuso
2023-09-12  8:32   ` Linux regression tracking (Thorsten Leemhuis)
2023-09-12 10:27     ` Florian Westphal
2023-09-12 11:47       ` Timo Sigurdsson
2023-09-12 11:52         ` Florian Westphal
2023-09-29 11:46       ` Linux regression tracking (Thorsten Leemhuis)
2023-09-12 11:39   ` Timo Sigurdsson
2023-09-12 11:58     ` Pablo Neira Ayuso
2023-09-12 19:13     ` Salvatore Bonaccorso
2023-09-15 20:44       ` Timo Sigurdsson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).