Re: netfilter: nf_conncount: cpu soft lockup using limiting with Open vSwitch.

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

On 12/9/25 9:42 AM, Pablo Neira Ayuso wrote:
> On Tue, Dec 09, 2025 at 08:57:59AM +0100, Fernando Fernandez Mancera wrote:
>> On 12/8/25 1:27 PM, Odintsov Vladislav wrote:
>>> On 08.12.2025 15:06, Rukomoinikova Aleksandra wrote:
>>>> Hi!
>>>> I was testing conntrack limiting using Open vSwitch and noticed the
>>>> following issue: under certain limits, a CPU lock occurred.
>>>>
>>>> [  491.682936] watchdog: BUG: soft lockup - CPU#1 stuck for 26s! [ovs-
>>>> dpctl:19437]
>>>>
>>>> This occurs during a high packet frequency when trying to get the set
>>>> limits through ovs-dpctl ct-get-limits.
>>>>
>>>> In the trace, I can see that the lock occurred on attempts to acquire a
>>>> spinlock.
>>>>
>>>> [  491.683056]  <IRQ>
>>>> [  491.683059]  _raw_spin_lock_bh+0x29/0x30
>>>> [  491.683064]  count_tree+0x19b/0x1f0 [nf_conncount]
>>>> [  491.683069]  ovs_ct_commit+0x196/0x490 [openvswitch]
>>>>
>>>> Prior to this, in the trace, there was processing of a task from
>>>> userspace (ovs-dpctl)
>>>>
>>>> [  491.683236]  </IRQ>
>>>> [  491.683237]  <TASK>
>>>> [  491.683238]  asm_common_interrupt+0x22/0x40
>>>> [  491.683240] RIP: 0010:nf_conncount_gc_list+0x18a/0x200 [nf_conncount]
>>>>
>>>> Inside the nf_conncount_gc_list function, a lock is taken on
>>>> nf_conncount.c:spin_trylock_bh(&list->list_lock):335. After this, the
>>>> not-so-fast __nf_conncount_gc_list function is executed. If, at this
>>>> moment, a packet interrupt arrives on the same сpu core (and
>>>> spin_trylock_bh doesn't disable interrupts on that core), then scenario
>>>> I encountered occurs: the first lock remains held, while the packet
>>>> interrupt also attempts to acquire it at
>>>> nf_conncount.c:spin_lock_bh(&rbconn->list.list_lock):502 while
>>>> committing to conntrack. This attempt fails, leading to a soft lockup.
>>>>
>>
>> Yes that makes sense. That nf_conncount_gc_list() was added there to cover a
>> different scenario which might be also affected by this soft lockup under
>> the same conditions.
> 
> See below, a quick browsing tells me OVS forgot to disable BH to
> perform this GC.
> 
>>>> Hence my question: shouldn't we avoid calling nf_conncount_gc_list when
>>>> querying limits without an skb (as OVS does in openvswitch/
>>>> conntrack.c:1773)? The limit retrieval operation should be read-only
>>>> regarding the contract state, not involve potential modification.
>>>>
>>>> Like this:
>>>> --- a/net/netfilter/nf_conncount.c
>>>> +++ b/net/netfilter/nf_conncount.c
>>>> @@ -495,7 +495,6 @@ count_tree(struct net *net,
>>>>                 int ret;
>>>>
>>>>                 if (!skb) {
>>>> -                nf_conncount_gc_list(net, &rbconn->list);
>>>>                     return rbconn->list.count;
>>>>                 }
>>>>
>>
>> Let me think on something, I would like to provide a solution that is
>> suitable for OVS + xt/nft_connlimit. Because this change would break some
>> xt_connlimit use-cases. Also without this nf_conncount_gc_list(), the
>> connection count wouldn't be accurate.. if some connections closed already
>> the count number would still consider them..
> 
> Side note, this particular line only affects OVS, which is the only
> caller passing NULL as skb:
> 
> net/netfilter/xt_connlimit.c:   connections = nf_conncount_count_skb(net, skb, xt_family(par), info->data, key);
> net/openvswitch/conntrack.c:    connections = nf_conncount_count_skb(net, skb, info->family,
> net/openvswitch/conntrack.c:    zone_limit.count = nf_conncount_count_skb(net, NULL, 0, data,
> 
> Another relevant aspect: nf_conncount_gc_list() is called _without_
> disabling BH (before recent Fernando's changes).
> 
> You fix it here, Fernando:
> 
> commit c0362b5748282e22fa1592a8d3474f726ad964c2
> Author: Fernando Fernandez Mancera <fmancera@suse.de>
> Date:   Fri Nov 21 01:14:31 2025 +0100
>   
>      netfilter: nf_conncount: make nf_conncount_gc_list() to disable BH
> 
> I think it is only a matter of backporting it to -stable.

That is right, thanks Pablo. Just a note, that commit doesn't have a 
fixes tag because I just did it to simplify its use so it won't be 
picked automatically.. should we send a request to stable mailing list?

Thanks,
Fernando.