[PATCH nf-next v2 1/2] netfilter: nf_queue: limit the hook drop flush to the unregistered hook point

[PATCH nf-next v2 1/2] netfilter: nf_queue: limit the hook drop flush to the unregistered hook point

[Antonio Ojea]

Unregistering a netfilter hook calls nf_queue_nf_hook_drop(), which makes
nfqnl_nf_hook_drop() walk every nfqueue instance of the network namespace
and flush it, reinjecting every packet that waits for a verdict with
NF_DROP. Deleting an nftables base chain of any table, family and hook
point therefore drops the packets queued by hooks of unrelated tables,
families and hook points. The verdict that userspace sends for them
afterwards fails with -ENOENT.

Before commit 039b40ee5854 ("netfilter: nf_queue: only call
synchronize_net twice if nf_queue is active") nf_queue_nf_hook_drop()
received the removed hook and nfqueue only dropped the entries queued by
that hook. The commit removed the argument and the comparison because
base chain removal while nfqueue is in use was rare at the time. With
base chains added and removed by userspace agents while another program
holds packets in a queue this is no longer the case. Commit
960632ece694 ("netfilter: convert hook list to an array") then replaced
the hook pointer in struct nf_queue_entry with an index into the hook
array of the hook point, so a comparison per hook is no longer possible.
A comparison per hook point is: the index of an entry is only stale when
the array of its own hook point changed.

Pass the nf_hook_ops being unregistered down to the queue handler and
add nf_hook_cmp() to nfnetlink_queue, a cmpfn for nfqnl_flush() that
matches the entries whose state.pf and state.hook are the hook point of
that hook. The handler takes the nf_hook_ops rather than the family of
the modified array to keep a single argument with NULL meaning "flush
everything", which nf_ct_iterate_destroy() uses on module exit and
__nft_ct_set_destroy() uses when a ct zone expression goes away and a
queued packet may still reference its conntrack template. The
cmpfn resolves the family itself. An NFPROTO_INET hook is registered in
the IPv4 and the IPv6 array, so it matches the entries of both families;
nf_unregister_net_hook() calls __nf_unregister_net_hook() for the two
arrays back to back and the second flush finds nothing left. If the
shrink of one array fails to allocate, that array keeps its dummy hook
and its entries stay valid, so the flush for the other family drops
them without need; no entry with a stale index survives. An
NFPROTO_INET hook at NF_INET_INGRESS is registered in the netdev ingress
array, the same mapping that nf_static_key_inc() applies. nfqueue does
not accept the ingress and egress hooks (nft_queue_validate()), the
mapping keeps the comparison consistent with the registration.

The entries queued from the hook point whose array changed are still
dropped, their hook_index is stale. Registering a hook in front of the
one that queued a packet still makes nf_reinject() resume at a stale
index, so the packet traverses the queueing hook a second time; this is
not changed here.

Tested with the reproducer script from the thread, unchanged: it queues
20 UDP packets from an inet output chain to a queue program that delays
every verdict by 100 ms and changes the ruleset while they wait. Before
this change deleting an unrelated ip6 base chain or an unrelated ip6
table delivers 3 of 20 packets and the queue program exits on ENOENT;
with it all 20 are delivered. Deleting a base chain at the same hook
point (inet output, another priority, in the same or another table) or
an ip table with an output base chain still drops the queued packets;
deleting an ip6 table with an output base chain or a netdev ingress
chain does not. Adding a base chain in front of the queueing one still
queues the packets twice. The nft_queue.sh selftest passes, with
CONFIG_NETFILTER_NETLINK_QUEUE=y and built as a module. The following
patch adds a test for both outcomes that holds the verdicts instead of
delaying them.

The code and this commit message were written with an LLM coding
assistant from a task description that contained the report, the
suggestion from the thread and the two commits above. The author
reviewed the result, directed the changes and validated them with the
reproducer and the selftest in virtme-ng before and after the change.

Suggested-by: Florian Westphal <fw@strlen.de>
Link: https://lore.kernel.org/netfilter-devel/CABhP=tYtCyr-qzL==98nkmDVZr6x+f8E26V1AAOpMXgZgt0vAQ@mail.gmail.com/
Assisted-by: LLM
Signed-off-by: Antonio Ojea <aojea@google.com>
---
v1: https://lore.kernel.org/netfilter-devel/20260926100351.2987307-1-aojea@google.com/

Changes in v2:
- rebase on nf-next. Convert the nf_queue_nf_hook_drop() call that commit
  36eae0956f65 ("netfilter: nft_ct: drop pending enqueued packets on
  removal") added to __nft_ct_set_destroy(); it passes NULL and keeps
  flushing every queued packet. Mention this caller in the commit message
  and in the comment on the NULL case.
  https://lore.kernel.org/netfilter-devel/arp1eSHB_m2cFy6n@strlen.de/

 include/net/netfilter/nf_queue.h  |  3 +-
 net/netfilter/core.c              |  2 +-
 net/netfilter/nf_conntrack_core.c |  2 +-
 net/netfilter/nf_internals.h      |  2 +-
 net/netfilter/nf_queue.c          |  4 +--
 net/netfilter/nfnetlink_queue.c   | 49 +++++++++++++++++++++++++++++--
 net/netfilter/nft_ct.c            |  2 +-
 7 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index fc3e81c07364..86021c55ba3c 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -35,7 +35,8 @@ struct nf_queue_entry {
 struct nf_queue_handler {
 	int		(*outfn)(struct nf_queue_entry *entry,
 				 unsigned int queuenum);
-	void		(*nf_hook_drop)(struct net *net);
+	void		(*nf_hook_drop)(struct net *net,
+					const struct nf_hook_ops *ops);
 };
 
 void nf_register_queue_handler(const struct nf_queue_handler *qh);
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 675a1034b340..9351b480115d 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -515,7 +515,7 @@ static void __nf_unregister_net_hook(struct net *net, int pf,
 	if (!p)
 		return;
 
-	nf_queue_nf_hook_drop(net);
+	nf_queue_nf_hook_drop(net, reg);
 	nf_hook_entries_free(p);
 }
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index d0d9e5ea84a0..37dbd89ac21e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2411,7 +2411,7 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
 
 		if (atomic_read(&cnet->count) == 0)
 			continue;
-		nf_queue_nf_hook_drop(net);
+		nf_queue_nf_hook_drop(net, NULL);
 	}
 	up_read(&net_rwsem);
 
diff --git a/net/netfilter/nf_internals.h b/net/netfilter/nf_internals.h
index 25403023060b..ce5aff2cae71 100644
--- a/net/netfilter/nf_internals.h
+++ b/net/netfilter/nf_internals.h
@@ -24,7 +24,7 @@
 #define CTA_FILTER_FLAG(ctattr) CTA_FILTER_F_ ## ctattr
 
 /* nf_queue.c */
-void nf_queue_nf_hook_drop(struct net *net);
+void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_ops *ops);
 
 /* nf_log.c */
 int __init netfilter_log_init(void);
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 73363ceedebe..166d05e994ac 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -129,14 +129,14 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 }
 EXPORT_SYMBOL_GPL(nf_queue_entry_get_refs);
 
-void nf_queue_nf_hook_drop(struct net *net)
+void nf_queue_nf_hook_drop(struct net *net, const struct nf_hook_ops *ops)
 {
 	const struct nf_queue_handler *qh;
 
 	rcu_read_lock();
 	qh = rcu_dereference(nf_queue_handler);
 	if (qh)
-		qh->nf_hook_drop(net);
+		qh->nf_hook_drop(net, ops);
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(nf_queue_nf_hook_drop);
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index c727668b0c5b..0b1a0e287aea 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1559,9 +1559,47 @@ static struct notifier_block nfqnl_dev_notifier = {
 	.notifier_call	= nfqnl_rcv_dev_event,
 };
 
-static void nfqnl_nf_hook_drop(struct net *net)
+/* Match the entries whose hook_index points into the hook array that
+ * __nf_unregister_net_hook() replaces when it removes @ops.
+ *
+ * ops->pf and ops->hooknum name the hook point as it was registered.
+ * entry->state.pf and entry->state.hook name the array the packet was
+ * traversing when it was queued.  They differ where one registration
+ * covers a different array than its name says, see nf_hook_entry_head():
+ *
+ *   NFPROTO_INET, hooknum != NF_INET_INGRESS: hooks_ipv4[] and hooks_ipv6[],
+ *   entries carry NFPROTO_IPV4 or NFPROTO_IPV6.
+ *   NFPROTO_INET, NF_INET_INGRESS: dev->nf_hooks_ingress, the array of the
+ *   NFPROTO_NETDEV/NF_NETDEV_INGRESS hook point, entries carry that pair.
+ *
+ * Every other family uses the array of its own name.
+ */
+static int nf_hook_cmp(struct nf_queue_entry *entry, unsigned long ops_ptr)
+{
+	const struct nf_hook_ops *ops = (const struct nf_hook_ops *)ops_ptr;
+	unsigned int hooknum = ops->hooknum;
+	u8 pf = ops->pf;
+
+	/* same translation as nf_static_key_inc() */
+	if (pf == NFPROTO_INET && hooknum == NF_INET_INGRESS) {
+		pf = NFPROTO_NETDEV;
+		hooknum = NF_NETDEV_INGRESS;
+	}
+
+	if (entry->state.hook != hooknum)
+		return 0;
+
+	if (pf == NFPROTO_INET)
+		return entry->state.pf == NFPROTO_IPV4 ||
+		       entry->state.pf == NFPROTO_IPV6;
+
+	return entry->state.pf == pf;
+}
+
+static void nfqnl_nf_hook_drop(struct net *net, const struct nf_hook_ops *ops)
 {
 	struct nfnl_queue_net *q = nfnl_queue_pernet(net);
+	nfqnl_cmpfn cmpfn = NULL;
 	int i;
 
 	/* This function is also called on net namespace error unwind,
@@ -1574,12 +1612,19 @@ static void nfqnl_nf_hook_drop(struct net *net)
 	if (!q)
 		return;
 
+	/* ops is NULL when the caller is not unregistering a hook but
+	 * removing an object queued packets may reference, every entry
+	 * has to go
+	 */
+	if (ops)
+		cmpfn = nf_hook_cmp;
+
 	for (i = 0; i < INSTANCE_BUCKETS; i++) {
 		struct nfqnl_instance *inst;
 		struct hlist_head *head = &q->instance_table[i];
 
 		hlist_for_each_entry_rcu(inst, head, hlist)
-			nfqnl_flush(inst, NULL, 0);
+			nfqnl_flush(inst, cmpfn, (unsigned long)ops);
 	}
 }
 
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 3c4c2faa7398..5a8db5592036 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -542,7 +542,7 @@ static void __nft_ct_set_destroy(const struct nft_ctx *ctx, struct nft_ct *priv)
 #endif
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 	case NFT_CT_ZONE:
-		nf_queue_nf_hook_drop(ctx->net);
+		nf_queue_nf_hook_drop(ctx->net, NULL);
 		mutex_lock(&nft_ct_pcpu_mutex);
 		if (--nft_ct_pcpu_template_refcnt == 0)
 			nft_ct_tmpl_put_pcpu();

base-commit: 87b80c2f6b05cad9f0ff9136709c62a0f59923e3
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH nf-next v2 2/2] selftests: netfilter: nft_queue: check the scope of the hook drop flush

[Antonio Ojea]

Add test_hook_drop_scope. It queues 20 UDP packets from an inet output
chain to a queue program that holds all verdicts, deletes a base chain
while they wait, and reads the queue length from
/proc/net/netfilter/nfnetlink_queue right after the deletion. It then
releases the queue program and checks with a counter in the input
chain and the packet total of the queue program that the surviving
packets are delivered.

Deleting a base chain at another hook point (ip6 prerouting) must keep
the 20 queued packets. Deleting a base chain at the queueing hook point
(inet output, another priority) drops them, because the index of a
queued packet into the hook array of its hook point is stale once that
array changed. The second case documents the remaining behaviour so
that a change to it is visible.

To hold the packets without depending on timing, add the -H option to
the nf_queue helper: it blocks SIGUSR1, reads the first packet and
waits in sigwait() until it receives SIGUSR1, then sends the verdicts
as usual. The test sends the signal after the ruleset change, so the
queue length it reads is exact on slow machines too.

Without the previous patch the first case fails: 0 of 20 packets remain
queued after the deletion and none is delivered.

The test and the helper option were written with an LLM coding
assistant from a task description; the author reviewed the result,
and validated the test in virtme-ng on kernels with and without the
previous patch.

Assisted-by: LLM
Signed-off-by: Antonio Ojea <aojea@google.com>
---
v1: https://lore.kernel.org/netfilter-devel/20260926100351.2987307-2-aojea@google.com/

Changes in v2:
- rebase on nf-next; nf_queue.c gained the -o, -O and -b options, -H is
  added next to them.

 .../selftests/net/netfilter/nf_queue.c        |  22 +++-
 .../selftests/net/netfilter/nft_queue.sh      | 108 ++++++++++++++++++
 2 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/netfilter/nf_queue.c b/tools/testing/selftests/net/netfilter/nf_queue.c
index 8bbec37f5356..cca6c3cfa9e4 100644
--- a/tools/testing/selftests/net/netfilter/nf_queue.c
+++ b/tools/testing/selftests/net/netfilter/nf_queue.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <errno.h>
+#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -21,6 +22,7 @@ struct options {
 	bool failopen;
 	bool out_of_order;
 	bool bogus_verdict;
+	bool hold;
 	int verbose;
 	unsigned int queue_num;
 	unsigned int timeout;
@@ -34,6 +36,7 @@ static struct options opts;
 static void help(const char *p)
 {
 	printf("Usage: %s [-c|-v [-vv] ] [-o] [-O] [-b] [-t timeout] [-q queue_num] [-Qdst_queue ] [ -d ms_delay ] [-G]\n", p);
+	printf("          [-H] (hold verdicts until SIGUSR1 is received)\n");
 }
 
 static int parse_attr_cb(const struct nlattr *attr, void *data)
@@ -280,6 +283,7 @@ static int mainloop(void)
 	uint32_t ooo_ids[16];
 	unsigned int portid;
 	int ooo_count = 0;
+	sigset_t hold_set;
 	char *buf;
 	int ret;
 
@@ -289,6 +293,12 @@ static int mainloop(void)
 		exit(EXIT_FAILURE);
 	}
 
+	/* -H: keep SIGUSR1 pending until the first packet is read */
+	sigemptyset(&hold_set);
+	sigaddset(&hold_set, SIGUSR1);
+	if (opts.hold)
+		sigprocmask(SIG_BLOCK, &hold_set, NULL);
+
 	nl = open_queue();
 	portid = mnl_socket_get_portid(nl);
 
@@ -320,6 +330,13 @@ static int mainloop(void)
 		}
 
 		id = ret - MNL_CB_OK;
+		if (opts.hold) {
+			int sig;
+
+			sigwait(&hold_set, &sig);
+			opts.hold = false;
+		}
+
 		if (opts.delay_ms)
 			sleep_ms(opts.delay_ms);
 
@@ -364,7 +381,7 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "chvoObt:q:Q:d:G")) != -1) {
+	while ((c = getopt(argc, argv, "chvoObt:q:Q:d:GH")) != -1) {
 		switch (c) {
 		case 'c':
 			opts.count_packets = true;
@@ -373,6 +390,9 @@ static void parse_opts(int argc, char **argv)
 			help(argv[0]);
 			exit(0);
 			break;
+		case 'H':
+			opts.hold = true;
+			break;
 		case 'q':
 			opts.queue_num = atoi(optarg);
 			if (opts.queue_num > 0xffff)
diff --git a/tools/testing/selftests/net/netfilter/nft_queue.sh b/tools/testing/selftests/net/netfilter/nft_queue.sh
index 7c857a2e0f34..01723c0f3e31 100755
--- a/tools/testing/selftests/net/netfilter/nft_queue.sh
+++ b/tools/testing/selftests/net/netfilter/nft_queue.sh
@@ -861,6 +861,113 @@ test_queue_bridge()
 	test_queue 20 "bridge"
 }
 
+hook_drop_delivered()
+{
+	ip netns exec "$ns1" nft list counter inet nfqscope delivered |
+		sed -n 's/.*packets \([0-9]*\) .*/\1/p'
+}
+
+hook_drop_queued()
+{
+	ip netns exec "$ns1" awk '$1 == 1 { print $3 }' /proc/self/net/netfilter/nfnetlink_queue
+}
+
+hook_drop_all_queued()
+{
+	[ "$(hook_drop_queued)" = "$1" ]
+}
+
+# hook_drop_case <keep|drop> <description> <nft command...>
+#
+# 1. The inet output chain sends udp packets to port 12345 to queue 1,
+#    the inet input chain counts them once they are accepted.
+# 2. nf_queue -H reads the packets but sends no verdict until it gets
+#    SIGUSR1, so the 20 packets sent stay in the kernel queue.
+# 3. The nft command removes a base chain while they are queued.  The
+#    queue length in /proc right after it tells if the kernel dropped
+#    them: still 20, or 0.
+# 4. SIGUSR1 releases nf_queue.  If the kernel kept the packets they are
+#    accepted now and reach the input counter.  If it dropped them the
+#    first verdict fails with ENOENT and nothing is counted.
+hook_drop_case()
+{
+	local expect="$1"
+	local desc="$2"
+	local packets=20
+	local delivered queued result
+	shift 2
+
+ip netns exec "$ns1" nft -f /dev/stdin <<EOF
+flush ruleset
+table inet nfqscope {
+	chain output {
+		type filter hook output priority 0; policy accept;
+		udp dport 12345 queue num 1
+	}
+	chain input {
+		type filter hook input priority 0; policy accept;
+		udp dport 12345 counter name delivered
+	}
+	chain output2 {
+		type filter hook output priority 100; policy accept;
+	}
+	counter delivered {}
+}
+table ip6 unrelated {
+	chain prerouting {
+		type filter hook prerouting priority 0; policy accept;
+	}
+}
+EOF
+	ip netns exec "$ns1" ./nf_queue -c -q 1 -H -t "$timeout" > "$TMPFILE1" 2>&1 &
+	local nfqpid=$!
+
+	busywait "$BUSYWAIT_TIMEOUT" nf_queue_wait "$ns1" 1
+
+	ip netns exec "$ns1" bash -c \
+		"for i in \$(seq $packets); do echo x > /dev/udp/127.0.0.1/12345; done"
+
+	busywait "$BUSYWAIT_TIMEOUT" hook_drop_all_queued "$packets"
+
+	ip netns exec "$ns1" nft "$@"
+	queued=$(hook_drop_queued)
+
+	kill -USR1 "$nfqpid"
+	wait "$nfqpid"
+	delivered=$(hook_drop_delivered)
+
+	if [ "$queued" = "$packets" ] && [ "$delivered" = "$packets" ] &&
+	   grep -q "^$packets packets total" "$TMPFILE1"; then
+		result="keep"
+	elif [ "$queued" = "0" ] && [ "$delivered" = "0" ]; then
+		result="drop"
+	else
+		result="inconsistent"
+	fi
+
+	if [ "$result" = "$expect" ]; then
+		echo "PASS: $desc: queued packets $result"
+	else
+		echo "FAIL: $desc: queued packets $result, expected $expect" 1>&2
+		echo "      $queued of $packets queued after the change, $delivered delivered" 1>&2
+		ret=1
+	fi
+}
+
+test_hook_drop_scope()
+{
+	# the hook array of another hook point changes, queued packets stay
+	hook_drop_case keep "base chain removed at another hook point" \
+		delete chain ip6 unrelated prerouting
+
+	# the hook array of the queueing hook point changes, the position of
+	# the queued packets in it is stale, they are dropped
+	hook_drop_case drop "base chain removed at the queueing hook point" \
+		delete chain inet nfqscope output2
+
+	ip netns exec "$ns1" nft flush ruleset
+}
+
 ip netns exec "$nsrouter" sysctl net.ipv6.conf.all.forwarding=1 > /dev/null
 ip netns exec "$nsrouter" sysctl net.ipv4.conf.veth0.forwarding=1 > /dev/null
 ip netns exec "$nsrouter" sysctl net.ipv4.conf.veth1.forwarding=1 > /dev/null
@@ -909,6 +1016,7 @@ test_queue_stress
 # should be last, adds vrf device in ns1 and changes routes
 test_icmp_vrf
 test_queue_removal
+test_hook_drop_scope
 
 # turns router into a bridge
 test_queue_bridge
-- 
2.56.0.rc1.315.gc6ed9934b7-goog