netfilter: nf_queue: limit the hook drop flush to the unregistered hook point

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>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

1 files changed, 2 insertions, 1 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);