nf_queue: hook changes drop or re-queue packets waiting in any nfqueue

nf_queue: hook changes drop or re-queue packets waiting in any nfqueue

[Antonio Ojea]

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

Hi,

deleting an nftables base chain drops every packet that is waiting for
a verdict in any nfqueue of the network namespace, including packets
queued by hooks of other tables and other families. Adding a base chain
in front of the one that queued a packet makes the packet traverse the
queuing hook again after the verdict, so userspace sees it twice.

We found the first problem in kube-network-policies and kindnet
(Kubernetes network policy and CNI agents built on nfqueue) [1]. Their
nftables sync did "add table; delete table; add table" to replace the
ruleset, and every sync lost the connections being evaluated at that
moment. The userspace symptom is the verdict for the dropped packet
failing asynchronously with -ENOENT:

  "Could not receive message"
  error="netlink receive: no such file or directory"

The add/delete/add idiom was my mistake. The documented way to replace
a ruleset atomically is "flush table" (or "flush chain") in the same
transaction, which keeps the base chains and their hooks, and we need
to change that. However, it does not cover upgrades. A table written by an
older version may have a chain and/or sets the new version
no longer uses. Removing them still requires deleting the table, or
the chain, at least once at startup, with the same effect on every
nfqueue in the namespace. Suggestions on how to handle that case are
welcome.

Independently of fixing it in our projects, any other component will
trigger the same problem that applies to any nfqueue consumer.

Analysis assisted by AI (v6.18-rc1, unchanged in current mainline)
--------------------------------------------------

Deleting a base chain ends in __nf_unregister_net_hook(), which after
shrinking the hook array calls nf_queue_nf_hook_drop(net)
(net/netfilter/core.c:522). nfqnl_nf_hook_drop()
(net/netfilter/nfnetlink_queue.c:1184) walks every nfqnl_instance of
the namespace and calls nfqnl_flush(inst, NULL, 0), which reinjects all
entries with NF_DROP. Nothing in that path looks at which hook was
removed, which table owned it, or which hook queued each entry.

The reason is nf_queue_entry::hook_index: the entry stores its position
in the nf_hook_entries array of its hook point, and nf_reinject()
resumes the traversal from that index. Any change to the array makes
the index stale, so on unregister everything queued is dropped.

Registering a hook does not drop anything, but it also changes the
array. When the new hook sorts before the queuing one, nf_reinject()
finds the new hook at hook_index, does ++i, and nf_iterate() runs the
queuing hook again. The packet is queued a second time.

History:

  039b40ee5854 ("netfilter: nf_queue: only call synchronize_net twice
  if nf_queue is active"), v4.12, removed the nf_hook_cmp() filter from
  nfqnl_nf_hook_drop() so that only entries queued by the removed hook
  were dropped. The commit message says: "For the rare case of base
  chain being unregistered or module removal while nfqueue is in use
  the extra hiccup due to the packet drops isn't a big deal."

  960632ece694 ("netfilter: convert hook list to an array"), v4.14,
  replaced the hook pointer in the entry with hook_index.

With nftables base chains managed by userspace daemons, unregistering
a hook is no longer rare, at least not as rare as before, and it happens in
namespaces where unrelated nfqueue consumers are running.

Reproducer
----------

The attached script (AI assisted) uses the nf_queue helper from
tools/testing/selftests/net/netfilter (gcc -o nf_queue nf_queue.c -lmnl).
It queues 20 UDP packets to queue 1 from an inet output chain, the
helper delays every verdict by 100 ms so most packets are waiting when
the ruleset is changed, and it checks that all 20 are delivered and that
the helper received each one exactly once.

  $ sudo NF_QUEUE=./nf_queue ./nfqueue-hook-drop.sh
  PASS: no ruleset change
        sent 20, queued 20, delivered 20
  PASS: delete regular chain (no hook) of unrelated table
        sent 20, queued 20, delivered 20
  PASS: add base chain to unrelated table (ip6 input)
        sent 20, queued 20, delivered 20
  PASS: add base chain after the queuing one (output prio 100)
        sent 20, queued 20, delivered 20
  FAIL: add base chain before the queuing one (output prio raw)
        sent 20, queued 37, delivered 20
  FAIL: delete base chain of unrelated table (ip6 prerouting)
        sent 20, queued ?, delivered 3
        nf_queue (exit status 1): mnl_cb_run: No such file or directory
  FAIL: delete unrelated table (ip6, no queue rule)
        sent 20, queued ?, delivered 3
        nf_queue (exit status 1): mnl_cb_run: No such file or directory

The "unrelated" table is ip6 with a prerouting base chain and no queue
rule. Deleting it drops the packets queued by the inet output chain of
another table. The "before" case shows the 17 packets that were waiting
being queued a second time. Reproduced on 7.1.6; the Go reproducer in
[1] shows the same drops on 7.0.12, also when the table being recreated
is not the one with the queue rule.

I can turn this into a test case for nft_queue.sh if that helps.

Happy to test patches or to send a selftest.

Thanks,
Antonio

[1] https://github.com/kubernetes-sigs/kube-network-policies/issues/402
    (kernel call path and a standalone Go reproducer by Ben Dwyer,
    who found the root cause)

[-- Attachment #2: nfqueue-hook-drop.sh.txt --]
[-- Type: text/plain, Size: 4423 bytes --]

#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
#
# Packets waiting for a verdict in an nfqueue do not survive changes to the
# netfilter hook array of the network namespace:
#
#  - unregistering any hook (deleting any nftables base chain, even one of
#    an unrelated table or family) drops every packet queued in every
#    nfqueue instance of the namespace (nfqnl_nf_hook_drop()).
#  - registering a hook in front of the one that queued the packet makes
#    nf_reinject() resume at a stale index, so the packet traverses the
#    queuing hook again and is queued a second time.
#
# Needs the nf_queue helper from tools/testing/selftests/net/netfilter:
#   gcc -o nf_queue nf_queue.c -lmnl
#
# Usage: NF_QUEUE=./nf_queue ./nfqueue-hook-drop.sh

NF_QUEUE=${NF_QUEUE:-./nf_queue}
PACKETS=20
ret=0

ns="nfq-$(mktemp -u XXXXXX)"
tmp=$(mktemp)

cleanup() {
	ip netns pids "$ns" 2>/dev/null | xargs -r kill 2>/dev/null
	ip netns del "$ns" 2>/dev/null
	rm -f "$tmp"
}
trap cleanup EXIT

if [ ! -x "$NF_QUEUE" ]; then
	echo "SKIP: $NF_QUEUE not found, build tools/testing/selftests/net/netfilter/nf_queue.c" >&2
	exit 4
fi

ip netns add "$ns" || exit 4
ip -net "$ns" link set lo up

# The UDP packets to port 12345 that the namespace sends to itself are queued
# to queue 1 at the IPv4 output hook, without bypass so they wait for the
# verdict. The counter in the input chain tells how many were delivered.
# "unrelated" is a table of another family with a base chain and no queue rule.
setup_ruleset() {
	ip netns exec "$ns" nft -f - <<EOF
flush ruleset
table inet nfqtest {
	chain output {
		type filter hook output priority filter; policy accept;
		udp dport 12345 queue to 1
	}
	chain input {
		type filter hook input priority filter; policy accept;
		udp dport 12345 counter name delivered
	}
	counter delivered {}
}
table ip6 unrelated {
	chain prerouting {
		type filter hook prerouting priority filter; policy accept;
	}
	chain regular {
	}
}
EOF
}

delivered() {
	ip netns exec "$ns" nft list counter inet nfqtest delivered |
		sed -n 's/.*packets \([0-9]*\) .*/\1/p'
}

# queued is how many packets the helper received, printed by nf_queue -c on
# a clean exit. It exits early on ENOENT, a verdict for a packet the kernel
# already dropped, so the count is not available in that case.
queued() {
	sed -n 's/^\([0-9]*\) packets total$/\1/p' "$tmp"
}

nf_queue_wait() {
	for _ in $(seq 50); do
		if ip netns exec "$ns" grep -q '^ *1 ' /proc/net/netfilter/nfnetlink_queue; then
			return 0
		fi
		sleep 0.1
	done
	echo "FAIL: nf_queue did not bind queue 1" >&2
	exit 1
}

# run_case <description> <nft command...>
#
# Sends PACKETS packets while the helper delays every verdict by 100ms, so
# most of them are still waiting when the nft command runs, and checks that
# all of them are delivered and that each one was queued exactly once.
run_case() {
	local desc="$1"
	shift

	setup_ruleset || exit 1

	ip netns exec "$ns" "$NF_QUEUE" -c -q 1 -d 100 -t 2 >"$tmp" 2>&1 &
	local pid=$!
	nf_queue_wait

	ip netns exec "$ns" bash -c "
		for i in \$(seq $PACKETS); do echo x >/dev/udp/127.0.0.1/12345; done"
	sleep 0.3 # a few verdicts land, the rest of the packets wait in the queue

	ip netns exec "$ns" nft "$@" >/dev/null

	wait "$pid"
	local status=$? got_delivered got_queued
	got_delivered=$(delivered)
	got_queued=$(queued)

	if [ "$got_delivered" = "$PACKETS" ] && [ "$got_queued" = "$PACKETS" ]; then
		echo "PASS: $desc"
	else
		echo "FAIL: $desc"
		ret=1
	fi
	echo "      sent $PACKETS, queued ${got_queued:-?}, delivered $got_delivered"
	grep -v '^$' "$tmp" | grep -v '^hook\|packets total' | sed "s/^/      nf_queue (exit status $status): /"
}

run_case "no ruleset change" \
	list ruleset
run_case "delete regular chain (no hook) of unrelated table" \
	delete chain ip6 unrelated regular
run_case "add base chain to unrelated table (ip6 input)" \
	add chain ip6 unrelated input '{ type filter hook input priority filter; }'
run_case "add base chain after the queuing one (output prio 100)" \
	add chain inet nfqtest late '{ type filter hook output priority 100; }'
run_case "add base chain before the queuing one (output prio raw)" \
	add chain inet nfqtest early '{ type filter hook output priority raw; }'
run_case "delete base chain of unrelated table (ip6 prerouting)" \
	delete chain ip6 unrelated prerouting
run_case "delete unrelated table (ip6, no queue rule)" \
	delete table ip6 unrelated

exit $ret

Re: nf_queue: hook changes drop or re-queue packets waiting in any nfqueue

[Florian Westphal]

Antonio Ojea <antonio.ojea.garcia@gmail.com> wrote:
> deleting an nftables base chain drops every packet that is waiting for
> a verdict in any nfqueue of the network namespace, including packets
> queued by hooks of other tables and other families. Adding a base chain
> in front of the one that queued a packet makes the packet traverse the
> queuing hook again after the verdict, so userspace sees it twice.
> 
> We found the first problem in kube-network-policies and kindnet
> (Kubernetes network policy and CNI agents built on nfqueue) [1]. Their
> nftables sync did "add table; delete table; add table" to replace the
> ruleset, and every sync lost the connections being evaluated at that
> moment. The userspace symptom is the verdict for the dropped packet
> failing asynchronously with -ENOENT:
> 
>   "Could not receive message"
>   error="netlink receive: no such file or directory"
> 
> The add/delete/add idiom was my mistake. The documented way to replace
> a ruleset atomically is "flush table" (or "flush chain") in the same
> transaction, which keeps the base chains and their hooks, and we need
> to change that. However, it does not cover upgrades. A table written by an
> older version may have a chain and/or sets the new version
> no longer uses. Removing them still requires deleting the table, or
> the chain, at least once at startup, with the same effect on every
> nfqueue in the namespace. Suggestions on how to handle that case are
> welcome.
> 
> Independently of fixing it in our projects, any other component will
> trigger the same problem that applies to any nfqueue consumer.

I don't think this is fixable.  You could tell your coding assistant
to pass const struct nf_hook_ops *reg down into nf_queue_nf_hook_drop().

If thats not NULL, then pass it to a new nfqnl_cmpfn() that only
drops skbs when the struct nf_queue_entry shares same pf and
same hooknum as the one in nf_hook_ops arg.

That would limit the impact a bit.

[PATCH nf-next 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. 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>
---
 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   | 46 +++++++++++++++++++++++++++++--
 6 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
index 4aeffddb7586..494ca3b300cc 100644
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -30,7 +30,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 11a702065bab..97611e48d304 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -519,7 +519,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 344f88295976..4b27f8daf265 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2416,7 +2416,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 7f12e56e6e52..d9c805bc8d7c 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -112,14 +112,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 8b7b39d8a109..4e7ded0b0f3a 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1181,9 +1181,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,
@@ -1196,12 +1234,16 @@ static void nfqnl_nf_hook_drop(struct net *net)
 	if (!q)
 		return;
 
+	/* ops is NULL on module exit, 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);
 	}
 }
 

base-commit: 18a7e218cfcdca6666e1f7356533e4c988780b57
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH nf-next 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>
---
 .../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 9e56b9d47037..07bc2b319840 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>
@@ -18,6 +19,7 @@
 struct options {
 	bool count_packets;
 	bool gso_enabled;
+	bool hold;
 	int verbose;
 	unsigned int queue_num;
 	unsigned int timeout;
@@ -31,6 +33,7 @@ static struct options opts;
 static void help(const char *p)
 {
 	printf("Usage: %s [-c|-v [-vv] ] [-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)
@@ -273,6 +276,7 @@ static int mainloop(void)
 	struct mnl_socket *nl;
 	struct nlmsghdr *nlh;
 	unsigned int portid;
+	sigset_t hold_set;
 	char *buf;
 	int ret;
 
@@ -282,6 +286,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);
 
@@ -310,6 +320,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);
 
@@ -329,7 +346,7 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "chvt:q:Q:d:G")) != -1) {
+	while ((c = getopt(argc, argv, "chvt:q:Q:d:GH")) != -1) {
 		switch (c) {
 		case 'c':
 			opts.count_packets = true;
@@ -338,6 +355,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 6136ceec45e0..89a4dd61845e 100755
--- a/tools/testing/selftests/net/netfilter/nft_queue.sh
+++ b/tools/testing/selftests/net/netfilter/nft_queue.sh
@@ -622,6 +622,113 @@ EOF
 	fi
 }
 
+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
@@ -668,5 +775,6 @@ test_udp_ct_race
 # should be last, adds vrf device in ns1 and changes routes
 test_icmp_vrf
 test_queue_removal
+test_hook_drop_scope
 
 exit $ret
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

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

[Florian Westphal]

Antonio Ojea <aojea@google.com> wrote:
> 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.

Looks good, but please rebase on top of current nf-next, as-is this
patch doesn't compile (there is another nfqnl_nf_hook_drop() call that
needs to be converted to 2-args scheme.

2nd patch doesn't apply cleanly.