[iptables PATCH v2 0/3] Chain counter fixes here and there

[iptables PATCH v2 0/3] Chain counter fixes here and there

[Phil Sutter]

Resending patch 2 after the test I created for it in patch 3 exposed the
iptables-nft bug fixed in patch 1.

Phil Sutter (3):
  nft: Create builtin chains with counters enabled
  Revert "libiptc: fix wrong maptype of base chain counters on restore"
  tests: shell: Test chain policy counter behaviour

 iptables/nft.c                                | 14 ++--
 .../shell/testcases/chain/0007counters_0      | 78 +++++++++++++++++++
 libiptc/libiptc.c                             |  2 +-
 3 files changed, 87 insertions(+), 7 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/chain/0007counters_0

-- 
2.40.0

[iptables PATCH v2 1/3] nft: Create builtin chains with counters enabled

[Phil Sutter]

The kernel enables policy counters for nftables chains only if
NFTA_CHAIN_COUNTERS attribute is present. For this to be generated, one
has to set NFTNL_CHAIN_PACKETS and NFTNL_CHAIN_BYTES attributes in the
allocated nftnl_chain object.

The above happened for base chains only with iptables-nft-restore if
called with --counters flag. Since this is very unintuitive to users,
fix the situation by adding counters to base chains in any case.

Fixes: 384958620abab ("use nf_tables and nf_tables compatibility interface")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 326dc20b21d65..97fd4f49fdb4c 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -701,6 +701,9 @@ nft_chain_builtin_alloc(int family, const char *tname,
 
 	nftnl_chain_set_str(c, NFTNL_CHAIN_TYPE, chain->type);
 
+	nftnl_chain_set_u64(c, NFTNL_CHAIN_PACKETS, 0);
+	nftnl_chain_set_u64(c, NFTNL_CHAIN_BYTES, 0);
+
 	return c;
 }
 
@@ -961,6 +964,7 @@ static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
 				       int policy,
 				       const struct xt_counters *counters)
 {
+	static const struct xt_counters zero = {};
 	struct nftnl_chain *c;
 	const struct builtin_table *_t;
 	const struct builtin_chain *_c;
@@ -985,12 +989,10 @@ static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
 		return NULL;
 	}
 
-	if (counters) {
-		nftnl_chain_set_u64(c, NFTNL_CHAIN_BYTES,
-					counters->bcnt);
-		nftnl_chain_set_u64(c, NFTNL_CHAIN_PACKETS,
-					counters->pcnt);
-	}
+	if (!counters)
+		counters = &zero;
+	nftnl_chain_set_u64(c, NFTNL_CHAIN_BYTES, counters->bcnt);
+	nftnl_chain_set_u64(c, NFTNL_CHAIN_PACKETS, counters->pcnt);
 
 	return c;
 }
-- 
2.40.0

[iptables PATCH v2 2/3] Revert "libiptc: fix wrong maptype of base chain counters on restore"

[Phil Sutter]

This reverts commit 7c4d668c9c2ee007c82063b7fc784cbbf46b2ec4.

The change can't be right: A simple rule append call will reset all
built-in chains' counters. The old code works fine even given the
mentioned "empty restore" use-case, at least if counters don't change on
the fly in-kernel.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=912
Fixes: 7c4d668c9c2ee ("libiptc: fix wrong maptype of base chain counters on restore")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 libiptc/libiptc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 634f0bc76b91c..e475063367c26 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -822,7 +822,7 @@ static int __iptcc_p_del_policy(struct xtc_handle *h, unsigned int num)
 
 		/* save counter and counter_map information */
 		h->chain_iterator_cur->counter_map.maptype =
-						COUNTER_MAP_ZEROED;
+						COUNTER_MAP_NORMAL_MAP;
 		h->chain_iterator_cur->counter_map.mappos = num-1;
 		memcpy(&h->chain_iterator_cur->counters, &pr->entry->counters,
 			sizeof(h->chain_iterator_cur->counters));
-- 
2.40.0

[iptables PATCH v2 3/3] tests: shell: Test chain policy counter behaviour

[Phil Sutter]

Test the last two fixes in that area.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../shell/testcases/chain/0007counters_0      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/chain/0007counters_0

diff --git a/iptables/tests/shell/testcases/chain/0007counters_0 b/iptables/tests/shell/testcases/chain/0007counters_0
new file mode 100755
index 0000000000000..0b21a92663299
--- /dev/null
+++ b/iptables/tests/shell/testcases/chain/0007counters_0
@@ -0,0 +1,78 @@
+#!/bin/bash -e
+
+SETUP="*filter
+:FORWARD ACCEPT [13:37]
+-A FORWARD -c 1 2 -j ACCEPT
+-A FORWARD -c 3 4 -j ACCEPT
+COMMIT"
+
+
+### -Z with index shall zero a single chain only
+
+EXPECT="-P FORWARD ACCEPT -c 13 37
+-A FORWARD -c 0 0 -j ACCEPT
+-A FORWARD -c 3 4 -j ACCEPT"
+
+$XT_MULTI iptables-restore --counters <<< "$SETUP"
+$XT_MULTI iptables -Z FORWARD 1
+diff -u <(echo "$EXPECT") <($XT_MULTI iptables -vS FORWARD)
+
+
+### -Z without index shall zero the chain and all rules
+
+EXPECT="-P FORWARD ACCEPT -c 0 0
+-A FORWARD -c 0 0 -j ACCEPT
+-A FORWARD -c 0 0 -j ACCEPT"
+
+$XT_MULTI iptables -Z FORWARD
+diff -u <(echo "$EXPECT") <($XT_MULTI iptables -vS FORWARD)
+
+
+### prepare for live test
+
+# iptables-nft will create output chain on demand, so make sure it exists
+$XT_MULTI iptables -A OUTPUT -d 127.2.3.4 -j ACCEPT
+
+# test runs in its own netns, lo is there but down by default
+ip link set lo up
+
+
+### pings (and pongs) hit OUTPUT policy, its counters must increase
+
+get_pkt_counter() { # (CHAIN)
+	$XT_MULTI iptables -vS $1 | awk '/^-P '$1'/{print $5; exit}'
+}
+
+counter_inc_test() {
+	pkt_pre=$(get_pkt_counter OUTPUT)
+	ping -q -i 0.2 -c 3 127.0.0.1
+	pkt_post=$(get_pkt_counter OUTPUT)
+	[[ $pkt_post -gt $pkt_pre ]]
+}
+
+counter_inc_test
+
+# iptables-nft-restore needed --counters to create chains with them
+if [[ $XT_MULTI == *xtables-nft-multi ]]; then
+	$XT_MULTI iptables -F OUTPUT
+	$XT_MULTI iptables -X OUTPUT
+	$XT_MULTI iptables-restore <<EOF
+*filter
+:OUTPUT ACCEPT [0:0]
+COMMIT
+EOF
+	counter_inc_test
+fi
+
+### unrelated restore must not touch changing counters in kernel
+
+# With legacy iptables, this works without --noflush even. With iptables-nft,
+# ruleset is flushed though. Not sure which behaviour is actually correct. :)
+pkt_pre=$pkt_post
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter$(ping -i 0.2 -c 3 127.0.0.1 >/dev/null 2>&1)
+COMMIT
+EOF
+nft list ruleset
+pkt_post=$(get_pkt_counter OUTPUT)
+[[ $pkt_post -eq $((pkt_pre + 6 )) ]]
-- 
2.40.0

Re: [iptables PATCH v2 0/3] Chain counter fixes here and there

[Phil Sutter]

On Thu, Aug 10, 2023 at 01:23:22PM +0200, Phil Sutter wrote:
> Resending patch 2 after the test I created for it in patch 3 exposed the
> iptables-nft bug fixed in patch 1.
> 
> Phil Sutter (3):
>   nft: Create builtin chains with counters enabled
>   Revert "libiptc: fix wrong maptype of base chain counters on restore"
>   tests: shell: Test chain policy counter behaviour

Series applied.