Re: [PATCH net v3] openvswitch: fix soft lockup in the netlink flow dump

[Ilya Maximets <i.maximets@ovn.org>]

On 10/2/26 12:27 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] ovs_flow_cmd_dump() (net/openvswitch/datapath.c) now holds BH
>   disabled across the whole dump callback.
> 
> --
> 
> openvswitch: fix soft lockup in the netlink flow dump
> 
> This patch disables BH across the whole table walk in ovs_flow_cmd_dump().
> The goal is to stop the per-CPU spin_unlock_bh() calls in
> ovs_flow_stats_get() from running the softirq backlog in the dumping
> thread's context. That backlog processing led to a soft lockup on a
> heavily loaded compute node.
> 
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index 631a03136fa14..4fc5d0bebd85e 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -1532,6 +1532,12 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>  		return -ENODEV;
>>  	}
>>  
>> +	/*
>> +	 * Not needed for safety. Stops every spin_unlock_bh() in
>> +	 * ovs_flow_stats_get() from running the softirq backlog.
>> +	 */
>> +	local_bh_disable();
>> +
>>  	ti = rcu_dereference(dp->table.ti);
>>  	for (;;) {
>>  		struct sw_flow *flow;
> 
> [ ... ]
> 
>> @@ -1552,6 +1558,8 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>  		cb->args[0] = bucket;
>>  		cb->args[1] = obj;
>>  	}
>> +
>> +	local_bh_enable();
>>  	rcu_read_unlock();
>>  	return skb->len;
>>  }
> 
> [Severity: Medium]
> Can a namespace user control how long this BH-off region lasts?
> 
> The region now covers the whole walk in ovs_flow_tbl_dump_next(). When
> buckets are empty, that loop has no exit that depends on skb space:
> 
> net/openvswitch/flow_table.c:ovs_flow_tbl_dump_next() {
>     ...
> 	while (*bucket < ti->n_buckets) {
> 		i = 0;
> 		head = &ti->buckets[*bucket];
> 		hlist_for_each_entry_rcu(flow, head, flow_table.node[ver]) {
> 		...
> 		}
> 		(*bucket)++;
> 		*last = 0;
> 	}
>     ...
> }
> 
> This means one callback can scan the entire bucket array with softirqs held
> off.
> 
> The only cap on n_buckets is the kvmalloc limit, which allows about 2^27
> buckets:
> 
> net/openvswitch/flow_table.c:flow_key_insert() {
>     ...
> 	if (table->count > ti->n_buckets)
> 		new_ti = table_instance_expand(ti, false);
> 	else if (time_after(jiffies, table->last_rehash + REHASH_INTERVAL))
> 		new_ti = table_instance_rehash(ti, ti->n_buckets, false);
>     ...
> }
> 
> The table never gets smaller in normal use:
> 
>   - ovs_flow_tbl_remove() never shrinks it.
>   - The periodic rehash keeps the same size.
>   - Only ovs_flow_tbl_flush() goes back to TBL_MIN_BUCKETS.
> 
> OVS_DP_CMD_NEW and OVS_FLOW_CMD_NEW/DEL only need GENL_UNS_ADMIN_PERM. The
> dump needs no privilege at all:
> 
> net/openvswitch/datapath.c:dp_flow_genl_ops[] {
>     ...
> 	{ .cmd = OVS_FLOW_CMD_GET,
> 	  .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> 	  .flags = 0,		    /* OK for unprivileged users. */
> 	  .doit = ovs_flow_cmd_get,
> 	  .dumpit = ovs_flow_cmd_dump
> 	},
>     ...
> }
> 
> Root in an unprivileged user+net namespace could do the following:
> 
>   1. Create a datapath.
>   2. Insert N flows so that n_buckets grows to between N and 2N.
>   3. Delete every flow except one in a high bucket.
>   4. Run the dump repeatedly from threads pinned to each CPU.
> 
> Each dump could then walk up to 1 GB of hlist_heads.

I don't think this is a concern, as the user will have to allocate
hundreds of GBs of flows in the first place before freeing them,
which is likely a larger problem.

> During that walk,
> NET_RX, timers, tasklets and RCU callbacks are all deferred on that CPU. On
> CONFIG_PREEMPT (non-RT) kernels, the walk also can't be preempted.
> 
> Before this patch, softirqs could still run on hardirq exit during the walk,
> and PREEMPT_RCU kernels could preempt it. On PREEMPT_NONE/VOLUNTARY kernels
> the walk was already non-preemptible under rcu_read_lock(), so the only new
> cost there is the softirq deferral.
> 
> Container memcg limits don't cap how far the table can grow:
> 
>   - The sw_flow kmem_cache is created in ovs_flow_init() without
>     SLAB_ACCOUNT.
>   - The bucket array from table_instance_alloc() comes from unaccounted
>     kvmalloc memory.
I have a patch set in works to properly account memory towards the memcg
of the user.  That will cover this case.  However, it's likely a net-next
material as it changes how users need to manage their memory.

Best regards, Ilya Maximets.