Re: [PATCH net-next 04/15] netfilter: nf_queue: limit the hook drop flush to the unregistered hook point

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Use-after-free of module code: after this patch, `rmmod
  br_netfilter` no longer drops queued packets that point into…
- [Medium] Conntrack module unload can now stall on queued packets, and
  the commit message does not mention it.
- [Low] The commit message has no Fixes: tag.

--

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

This patch passes the nf_hook_ops being unregistered down to the nfqueue
handler. It adds nf_hook_cmp() so that nfqnl_nf_hook_drop() only flushes
queued entries whose state.pf/state.hook match the hook point of the
removed hook. Callers that pass NULL (nf_ct_iterate_destroy() and
__nft_ct_set_destroy()) still flush every entry.

Link: https://lore.kernel.org/netfilter-devel/CABhP=tYtCyr-qzL==98nkmDVZr6x+f8E26V1AAOpMXgZgt0vAQ@mail.gmail.com/

> 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.

[ ... ]

> 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>

[Severity: Low]
Should this have a Fixes: tag?

The commit message describes a user-visible problem: deleting an unrelated
base chain drops queued packets, and the next verdict fails with -ENOENT.
It also names 039b40ee5854 as the commit that removed the per-hook
comparison.

Without the trailer, stable maintainers may miss this fix. The message
also does not say the omission is deliberate.

> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 477fc632657f4..90d887546f7db 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -1566,9 +1566,39 @@ static struct notifier_block nfqnl_dev_notifier = {

[ ... ]

> +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;
> +}

[Severity: High]
After this change, can packets whose okfn points into br_netfilter.ko stay
queued after that module is unloaded?

br_netfilter calls NF_HOOK() at hook points where it registers no hook of
its own, and passes its own functions as okfn:

net/bridge/br_netfilter_hooks.c:br_nf_forward() {
    ...
	NF_HOOK(pf, NF_INET_FORWARD, state->net, NULL, skb,
		brnf_get_logical_dev(skb, state->in, state->net),
		parent,	br_nf_forward_finish);
    ...
}

br_nf_forward_arp() does the same at NFPROTO_ARP/NF_ARP_FORWARD.
br_nf_post_routing() does it at NF_INET_POST_ROUTING with
br_nf_dev_queue_xmit.

br_nf_ops only has the NFPROTO_BRIDGE hooks and the IPv4/IPv6
NF_INET_PRE_ROUTING hooks. On rmmod br_netfilter the teardown is:

br_netfilter_fini()
  unregister_pernet_subsys()
    brnf_exit_net()
      nf_unregister_net_hooks(net, br_nf_ops, ARRAY_SIZE(br_nf_ops))
        __nf_unregister_net_hook()
          nf_queue_nf_hook_drop(net, reg)
            nfqnl_nf_hook_drop()
              nf_hook_cmp()

Suppose a bridged packet is queued by an ip/ip6 FORWARD or POSTROUTING
NFQUEUE rule, or an arp FORWARD rule. nf_hook_cmp() then returns 0 for
every hook in br_nf_ops.

For example, the NFPROTO_BRIDGE NF_BR_FORWARD hook has the same hooknum as
NF_INET_FORWARD, but entry->state.pf is NFPROTO_IPV4 rather than
NFPROTO_BRIDGE.

Before this patch, each unregistration flushed every entry in the netns.
That flush is what removed these packets, since queue entries take no
module reference.

When userspace later sends NF_ACCEPT, the IPv4 FORWARD hook array has not
changed. hook_index is still valid, so nf_reinject() reaches:

	entry->state.okfn(entry->state.net, entry->state.sk, skb);

okfn is br_nf_forward_finish(), which is in the freed module text.

Is a flush-all, such as nf_queue_nf_hook_drop(net, NULL), needed in the
br_netfilter exit path, or some other flush at module teardown for callers
whose okfn is module code?

> +
> +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,
> @@ -1581,12 +1611,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);

[Severity: Medium]
Could rmmod nf_conntrack now block while userspace holds verdicts for
queued packets?

When the last conntrack user goes away, nf_ct_netns_do_put() unregisters
ipv4_conntrack_ops/ipv6_conntrack_ops. Those hooks are only at
PRE_ROUTING, LOCAL_IN, LOCAL_OUT and POST_ROUTING.

Before this patch, that unregistration flushed every queued entry. This
released the nf_conn references held through skb->_nfct.

With the filtered flush, packets queued at NF_INET_FORWARD stay queued, for
example from iptables -A FORWARD -j NFQUEUE or an nft forward queue rule.
Their conntracks keep cnet->count above zero.

nf_conntrack_standalone_fini() only calls nf_conntrack_cleanup_start() and
unregister_pernet_subsys(&nf_conntrack_net_ops). It never calls
nf_queue_nf_hook_drop(net, NULL). nf_conntrack_cleanup_net_list() then
keeps looping:

i_see_dead_people:
	busy = 0;
	list_for_each_entry(net, net_exit_list, exit_list) {
		...
		if (atomic_read(&cnet->count) != 0)
			busy = 1;
	}
	if (busy) {
		DEBUG_NET_WARN_ONCE(time_after(jiffies, start + 60 * HZ),
				    "conntrack cleanup blocked for 60s");
		schedule();
		goto i_see_dead_people;
	}

This runs while pernet_ops_rwsem is held for write. Netns creation and
teardown would stall until a verdict is sent or the queue socket is
closed.

The commit message says nf_ct_iterate_destroy() keeps the NULL flush on
module exit. That path covers helpers and NAT, not nf_conntrack itself,
and the message does not mention this case.

Should nf_conntrack's exit path also flush with NULL?

>  	}
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018005638.1776148-2-pablo%40netfilter.org