Re: [PATCH nf] netfilter: nft_connlimit: fix stale read of connection count

[Florian Westphal <fw@strlen.de>]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 10/24/25 1:31 PM, Florian Westphal wrote:
> > Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> >> nft_connlimit_eval() reads priv->list->count to check if the connection
> >> limit has been exceeded. This value can be cached by the CPU while it
> >> can be decremented by a different CPU when a connection is closed. This
> >> causes a data race as the value cached might be outdated.
> >>
> >> When a new connection is established and evaluated by the connlimit
> >> expression, priv->list->count is incremented by nf_conncount_add(),
> >> triggering the CPU's cache coherency protocol and therefore refreshing
> >> the cached value before updating it.
> >>
> >> Solve this situation by reading the value using READ_ONCE().
> > 
> > Hmm, I am not sure about this.
> > 
> > Patch looks correct (we read without holding a lock),
> > but I don't see how compiler would emit different code here.
> > 
> > This patch makes no difference on my end, same code is emitted.
> > 
> > Can you show code before and after this patch on your side?
> 
> I did `make net/netfilter/nft_connlimit.s` for before and after, then I 
> diffed both files with diff -u:
> 
> I see a difference on how count is being handled.

I'd apply this patch for correctness reasons. But I see no difference:

> @@ -984,19 +984,19 @@
>   # net/netfilter/nft_connlimit.c:46: 	if 
> (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
>   	testl	%eax, %eax	# _30
>   	jne	.L65	#,
> -# net/netfilter/nft_connlimit.c:51: 	count = priv->list->count;
> -	movq	8(%rbx), %rax	# MEM[(struct nft_connlimit *)expr_2(D) + 8B].list, 
> MEM[(struct nft_connlimit *)expr_2(D) + 8B].list
> +# net/netfilter/nft_connlimit.c:51: 	count = READ_ONCE(priv->list->count);
> +	movq	8(%rbx), %rax	# MEM[(struct nft_connlimit *)expr_2(D) + 8B].list, _31
> +	movl	88(%rax), %eax	# MEM[(const volatile unsigned int *)_31 + 88B], _32

old:
movq    8(%rbx), %rax  
movl    88(%rax), %eax
cmpl    %eax, 16(%rbx) 

new:
movq    8(%rbx), %rax
movl    88(%rax), %eax
cmpl    %eax, 16(%rbx)

same instruction sequence, only comments differ.
Doesn't invalidate the patch however, we do read while not holding
any locks so this READ_ONCE annotation is needed.