Re: [PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

This takes the NFNL_SUBSYS_QUEUE mutex around the instance destroy loop in
nfqnl_rcv_nl_event() so that the NETLINK_URELEASE notifier can no longer run
in parallel with nfqnl_recv_config().  The stale comment in nfqnl_recv_config()
is updated to describe the new serialization.

> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index c727668b0c5be..a3bc002800510 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
>  	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
>  		int i;
>  
> +		nfnl_lock(NFNL_SUBSYS_QUEUE);
>  		/* destroy all instances for this portid */
>  		spin_lock(&q->instances_lock);
>  		for (i = 0; i < INSTANCE_BUCKETS; i++) {

[Severity: Medium]
This is a pre-existing issue, not something introduced here, but does waiting
on the nfnl queue mutex here make it easier to destroy an instance that belongs
to a different socket?

The destroy loop in nfqnl_rcv_nl_event() picks victims by portid only:

	hlist_for_each_entry_safe(inst, t2, head, hlist) {
		if (n->portid == inst->peer_portid)
			__instance_destroy(inst);
	}

and net/netlink/af_netlink.c:netlink_release() makes the portid reusable well
before the notifier runs:

netlink_release()
	netlink_remove(sk);		/* portid is free for reuse here */
	sock_orphan(sk);
	...
	blocking_notifier_call_chain(&netlink_chain,
				     NETLINK_URELEASE, &n);

With the new nfnl_lock(NFNL_SUBSYS_QUEUE), the notifier now blocks for the
whole duration of any NFQNL_MSG_CONFIG handling (including instance_create()
and its sleeping rhashtable_init()), in any netns, since
net/netfilter/nfnetlink.c:nfnl_lock() takes the single global
table[NFNL_SUBSYS_QUEUE].mutex.  So this sequence looks possible:

thread 1: close() of a NETLINK_NETFILTER socket with portid X
	  netlink_remove() releases X, notifier is pending

thread 2: binds portid X, sends NFQNL_CFG_CMD_BIND
	  nfnetlink_rcv_msg() -> nfqnl_recv_config() under the queue mutex
	  instance_create() links a fresh instance with peer_portid == X

thread 1: notifier finally acquires the mutex, matches
	  inst->peer_portid == X and calls __instance_destroy() on the
	  brand new instance

Can the ownership check that the new comment relies on detect this?

	queue = instance_lookup(q, queue_num);
	if (queue && queue->peer_portid != NETLINK_CB(skb).portid) {

peer_portid matches in the recycled-portid case, so the check passes and the
instance is still torn down by the older socket's notification.

There is no memory safety problem, the instance is unlinked once and freed via
queue_rcu_work().  The visible effect is that a live socket's queue silently
disappears, its queued packets are flushed, packets hitting the NFQUEUE rule
then fail with -ESRCH (dropped unless fail-open or bypass is set), and later
CONFIG messages for that queue number return -ENODEV.  It requires
CAP_NET_ADMIN in the netns and a concurrent close/bind, and no later commit in
this series touches net/netfilter/nfnetlink_queue.c.

>  			}
>  		}
>  		spin_unlock(&q->instances_lock);
> +		nfnl_unlock(NFNL_SUBSYS_QUEUE);
>  	}
>  	return NOTIFY_DONE;
>  }
> @@ -1925,9 +1927,9 @@ static int nfqnl_recv_config(struct sk_buff *skb, const struct nfnl_info *info,
>  
>  	/* Lookup queue under RCU. After peer_portid check (or for new queue
>  	 * in BIND case), the queue is owned by the socket sending this message.
> -	 * A socket cannot simultaneously send a message and close, so while
> -	 * processing this CONFIG message, nfqnl_rcv_nl_event() (triggered by
> -	 * socket close) cannot destroy this queue. Safe to use without RCU.
> +	 * nfqnl_rcv_nl_event() will block on the nfnl subsys mutex that is
> +	 * held by the caller, so the queue cannot be destroyed in parallel,
> +	 * even after we drop the RCU read lock.
>  	 */

[ ... ]