Re: [PATCH net 09/12] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

Jakub Kicinski <[email protected]> wrote:
> 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
> 
> The NETLINK_URELEASE notifier now takes the NFNL_SUBSYS_QUEUE mutex around
> the instance teardown loop, so it can no longer run concurrently with
> nfqnl_recv_config().  The comment in nfqnl_recv_config() is updated to
> explain that the notifier blocks on the subsystem mutex held by the caller.
> 
> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index c727668b0c5b..a3bc00280051 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]
> The new mutex gives mutual exclusion, but does it give any ordering
> against the socket unhash?  In netlink_release() the socket is removed
> from the hash table before the notifier chain is invoked:

I'm useless.  I don't even know what this is trying to tell me.