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

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

On 10/24/25 1:02 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 10/24/25 1:20 AM, Fernando Fernandez Mancera 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().
>>>
>>> Fixes: df4a90250976 ("netfilter: nf_conncount: merge lookup and add functions")
>>> Closes: https://lore.kernel.org/netfilter/trinity-85c72a88-d762-46c3-be97-36f10e5d9796-1761173693813@3c-app-mailcom-bs12/
>>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>>> ---
>>
>> While at this, I have found another problem with connlimit although with
>> this fix, it is partially mitigated. Since d265929930e2 ("netfilter:
>> nf_conncount: reduce unnecessary GC"), if __nf_conncount_add() is called
>> more than once during the same jiffy, the function won't check if the
>> connection is already tracked and will be added right away incrementing
>> the count. This can cause a situation where the count is greater than it
>> should and can cause nft_connlimit to match wrongly for a few jiffies.
>>
>> I am open to suggestions on how to fix this.. as currently I don't have
>> a different one other than reverting the commit..
> 
> People are not supposed to use it in this way.
> 
> This is very expensive, there is a reason why iptables-extensions
> examples all use iptables --syn.
> 

If this is only supposed to be checked for SYN packets, that is, a new 
connection is started.. a simple fix could be to check whether this is a 
SYN packet or not, break if it isn't. Similar to what is done on 
synproxy eval function. What do you think?

> This needs a documentation fix.
> 
> Or, we could revert df4a90250976 and then only _add for ctinfo NEW.