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

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/24/25 2:49 PM, Florian Westphal wrote:
> 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.
> 


Yes, you are right. Let me send a V2 clarifying the commit message. I am 
sending some other patches as this requires other fixes..