Re: [PATCH nf,v3] netfilter: nfnetlink_queue: hash queue instance by portid too

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Sep 23, 2026 at 08:40:46PM +0200, Pablo Neira Ayuso wrote:
> netlink_release() calls netlink_remove() before delivering the
> NETLINK_URELEASE event. And a new queue instance can be created before
> delivering such NETLINK_URELEASE event. This allows for two different
> queue instances with the same portid to co-exist for a little while.
> Since the instance that is going away is destroyed by portid via netlink
> notifier, both the newly created instance and the one that is going away
> with the same portid are destroyed.

It seems I cannot fix this in this way. I forgot same socket can be
bound to several queues.

> Fixes: 7af4cc3fa158 ("[NETFILTER]: Add "nfnetlink_queue" netfilter queue handler over nfnetlink")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v3: - use siphash instead of lazy hash
>     - fix incorrect hlist and wrong instance_lookup_by_portid()
> 
>  net/netfilter/nfnetlink_queue.c | 36 +++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index c727668b0c5b..a2086c192063 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -69,6 +69,7 @@
>  
>  struct nfqnl_instance {
>  	struct hlist_node hlist;		/* global list of queues */
> +	struct hlist_node hlist_portid;		/* global list of queues, by portid */
>  	struct rhashtable nfqnl_packet_map;
>  	struct rcu_work	rwork;
>  
> @@ -96,11 +97,13 @@ typedef int (*nfqnl_cmpfn)(struct nf_queue_entry *, unsigned long);
>  
>  static struct workqueue_struct *nfq_cleanup_wq __read_mostly;
>  static unsigned int nfnl_queue_net_id __read_mostly;
> +static siphash_aligned_key_t nfnl_queue_hash_key;
>  
>  #define INSTANCE_BUCKETS	16
>  struct nfnl_queue_net {
>  	spinlock_t instances_lock;
>  	struct hlist_head instance_table[INSTANCE_BUCKETS];
> +	struct hlist_head instance_table_portid[INSTANCE_BUCKETS];
>  };
>  
>  static struct nfnl_queue_net *nfnl_queue_pernet(struct net *net)
> @@ -113,6 +116,15 @@ static inline u_int8_t instance_hashfn(u_int16_t queue_num)
>  	return ((queue_num >> 8) ^ queue_num) % INSTANCE_BUCKETS;
>  }
>  
> +static inline u8 instance_portid_hashfn(u32 portid)
> +{
> +	u64 hash = siphash_1u32(portid, &nfnl_queue_hash_key);
> +
> +	net_get_random_once(&nfnl_queue_hash_key, sizeof(nfnl_queue_hash_key));
> +
> +	return reciprocal_scale(hash, INSTANCE_BUCKETS);
> +}
> +
>  static const struct rhashtable_params nfqnl_rhashtable_params = {
>  	.head_offset = offsetof(struct nf_queue_entry, hash_node),
>  	.key_offset = offsetof(struct nf_queue_entry, id),
> @@ -136,6 +148,20 @@ instance_lookup(struct nfnl_queue_net *q, u_int16_t queue_num)
>  	return NULL;
>  }
>  
> +static struct nfqnl_instance *
> +instance_lookup_by_portid(struct nfnl_queue_net *q, u32 portid)
> +{
> +	struct nfqnl_instance *inst;
> +	struct hlist_head *head;
> +
> +	head = &q->instance_table_portid[instance_portid_hashfn(portid)];
> +	hlist_for_each_entry(inst, head, hlist_portid) {
> +		if (inst->peer_portid == portid)
> +			return inst;
> +	}
> +	return NULL;
> +}
> +
>  static struct nfqnl_instance *
>  instance_create(struct nfnl_queue_net *q, u_int16_t queue_num, u32 portid)
>  {
> @@ -160,7 +186,8 @@ instance_create(struct nfnl_queue_net *q, u_int16_t queue_num, u32 portid)
>  		goto out_free;
>  
>  	spin_lock(&q->instances_lock);
> -	if (instance_lookup(q, queue_num)) {
> +	if (instance_lookup(q, queue_num) ||
> +	    unlikely(instance_lookup_by_portid(q, portid))) {
>  		err = -EEXIST;
>  		goto out_unlock;
>  	}
> @@ -172,6 +199,8 @@ instance_create(struct nfnl_queue_net *q, u_int16_t queue_num, u32 portid)
>  
>  	h = instance_hashfn(queue_num);
>  	hlist_add_head_rcu(&inst->hlist, &q->instance_table[h]);
> +	h = instance_portid_hashfn(portid);
> +	hlist_add_head(&inst->hlist_portid, &q->instance_table_portid[h]);
>  
>  	spin_unlock(&q->instances_lock);
>  
> @@ -208,6 +237,7 @@ static void
>  __instance_destroy(struct nfqnl_instance *inst)
>  {
>  	hlist_del_rcu(&inst->hlist);
> +	hlist_del(&inst->hlist_portid);
>  
>  	INIT_RCU_WORK(&inst->rwork, instance_destroy_work);
>  	queue_rcu_work(nfq_cleanup_wq, &inst->rwork);
> @@ -2119,8 +2149,10 @@ static int __net_init nfnl_queue_net_init(struct net *net)
>  	unsigned int i;
>  	struct nfnl_queue_net *q = nfnl_queue_pernet(net);
>  
> -	for (i = 0; i < INSTANCE_BUCKETS; i++)
> +	for (i = 0; i < INSTANCE_BUCKETS; i++) {
>  		INIT_HLIST_HEAD(&q->instance_table[i]);
> +		INIT_HLIST_HEAD(&q->instance_table_portid[i]);
> +	}
>  
>  	spin_lock_init(&q->instances_lock);
>  
> -- 
> 2.47.3
> 
>