[PATCH v2] openvswitch: fix soft lockup in the netlink flow dump

[PATCH v2] openvswitch: fix soft lockup in the netlink flow dump

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

A production compute node carrying a few thousand datapath flows hit a
soft lockup inside a single netlink flow dump and panicked.

ovs_flow_cmd_dump() calls ovs_flow_stats_get() for every flow it
emits, and that releases stats->lock with spin_unlock_bh() once per
CPU that has touched the flow. Every release is a local_bh_enable(),
and each one runs the pending softirq backlog in the dumping thread's
own context.

The skb bounds how many flows one callback emits, and a large but
sparse table adds only a walk over empty buckets, so no dumper carries
a budget of its own. Neither bounds the softirq work the callback
absorbs. On a CPU that carries the box's packet load the backlog
refills as fast as it drains, so the dumping thread becomes that CPU's
softirq engine. It never sleeps and it has no reschedule point, so
under voluntary preemption nothing can take the CPU away from it:
neither the ksoftirqd the kernel woke to take the work over, nor the
stopper thread the softlockup detector dispatches to refresh its
timestamp.

Hold BH off across the whole callback instead, the way
ctnetlink_dump_table() does, so the nested spin_unlock_bh() stop
draining softirqs. The loop already runs under rcu_read_lock() and
cannot sleep. What it gives up is preemption under CONFIG_PREEMPT,
since a BH-off region is not preemptible outside PREEMPT_RT. That
region is bounded by the skb and the table size, where the softirq
backlog it used to absorb is not.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
v2:
- leave ovs_vport_cmd_dump() alone: nsid_lock has not been BH-safe
  since commit aed4969f2bdf ("net: net->nsid_lock does not need BH
  safety"), so the vport dump never drained softirqs
- disable BH before the table dereference and say in a comment that
  the region is not there for safety
- drop the ovs_flow_stats_get() history, note the empty-bucket walk
  and the lost CONFIG_PREEMPT preemption in the message
- move the Cc list out of the commit message, add the net prefix

 net/openvswitch/datapath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 631a03136fa1..a80bac81c043 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1532,6 +1532,11 @@ 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 here.
+	 */
+	local_bh_disable();
 	ti = rcu_dereference(dp->table.ti);
 	for (;;) {
 		struct sw_flow *flow;
@@ -1552,6 +1557,7 @@ 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;
 }
-- 
2.53.0

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The patch converts the whole netlink flow-dump callback into a
  softirq-disabled - and on !PREEMPT_RT CONFIG_PREEMPT kernels fully…
- [Low] The submission fixes a user-visible production failure (soft
  lockup and panic inside a single OVS_FLOW_CMD_GET netlink dump) but…

--

openvswitch: fix soft lockup in the netlink flow dump

The patch wraps the whole ovs_flow_cmd_dump() callback in
local_bh_disable()/local_bh_enable() so that the nested spin_unlock_bh()
calls inside ovs_flow_stats_get() no longer drain the softirq backlog in
the dumping thread's context.  The dump loop already runs under
rcu_read_lock() and does not sleep.

A couple of questions below.

[Severity: Low]
The change repairs a user-visible failure (soft lockup and panic in a
single OVS_FLOW_CMD_GET dump), but the trailer block carries only a
Signed-off-by, with no Fixes: tag and no stable indication.

Documentation/process/maintainer-netdev.rst says:

 - for fixes the ``Fixes:`` tag is required, regardless of the tree

The offending construct, the per-CPU spin_lock_bh()/spin_unlock_bh() pair
in ovs_flow_stats_get() executed once per CPU in flow->cpu_used_mask for
every emitted flow, is long standing, so the affected stable range is
wide.  Could a Fixes: tag (and a note on whether this is stable material)
be added so backporters know which trees are in scope?

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 631a03136fa14..a80bac81c0430 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1532,6 +1532,11 @@ 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 here.
> +	 */
> +	local_bh_disable();
>  	ti = rcu_dereference(dp->table.ti);
>  	for (;;) {
>  		struct sw_flow *flow;

[Severity: Medium]
Is the length of this new softirq-disabled region actually bounded by
anything small?

The commit message says:

    That region is bounded by the skb and the table size, where the
    softirq backlog it used to absorb is not.

The table size is the part that worries me.  ovs_flow_tbl_dump_next()
sweeps buckets with no per-callback budget:

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;
	}
}

and n_buckets only ever grows: table_instance_expand() doubles it when
table->count > ti->n_buckets, while ovs_flow_tbl_remove() and
table_instance_flow_free() only decrement table->count and never shrink
the bucket array.  So a datapath that once held a very large number of
flows keeps a large sparse table, and the last callback of a dump can
walk all remaining empty buckets in one BH-off region, with no
cond_resched() or other break point in the loop.

For every emitted flow the region also runs one lock/unlock pair per CPU:

net/openvswitch/flow.c:ovs_flow_stats_get() {
	for_each_cpu(cpu, flow->cpu_used_mask) {
		...
			spin_lock_bh(&stats->lock);
		...
			spin_unlock_bh(&stats->lock);
	}
}

While that region is held, NET_RX/NET_TX, timer and RCU softirqs are all
suppressed on the dumping CPU, so NAPI stalls and the rx backlog can
overflow.  The loop also loses the preemption it previously had inside the
preemptible rcu_read_lock() section on CONFIG_PREEMPT.

The dump is reachable without privileges:

	{ .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
	},

so a process can repeatedly force these windows on a host with a large
datapath table.

Would a per-callback budget work here, emitting N flows or scanning M
buckets and then returning so netlink re-invokes the callback with BH
re-enabled?  Alternatively, could the BH-off region be scoped to the
for_each_cpu() loop inside ovs_flow_stats_get() rather than the whole
callback?

I note ctnetlink_dump_table() already has the same whole-callback shape
over nf_conntrack_htable_size buckets, so the approach itself is not
unprecedented in net/; the question is only about bounding the OVS sweep.

> @@ -1552,6 +1557,7 @@ 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;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915122401.3910188-1-den%40openvz.org

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

[Ilya Maximets]

On 9/15/26 2:24 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> A production compute node carrying a few thousand datapath flows hit a
> soft lockup inside a single netlink flow dump and panicked.
> 
> ovs_flow_cmd_dump() calls ovs_flow_stats_get() for every flow it
> emits, and that releases stats->lock with spin_unlock_bh() once per
> CPU that has touched the flow. Every release is a local_bh_enable(),
> and each one runs the pending softirq backlog in the dumping thread's
> own context.
> 
> The skb bounds how many flows one callback emits, and a large but
> sparse table adds only a walk over empty buckets, so no dumper carries
> a budget of its own. Neither bounds the softirq work the callback
> absorbs. On a CPU that carries the box's packet load the backlog
> refills as fast as it drains, so the dumping thread becomes that CPU's
> softirq engine. It never sleeps and it has no reschedule point, so
> under voluntary preemption nothing can take the CPU away from it:
> neither the ksoftirqd the kernel woke to take the work over, nor the
> stopper thread the softlockup detector dispatches to refresh its
> timestamp.
> 
> Hold BH off across the whole callback instead, the way
> ctnetlink_dump_table() does, so the nested spin_unlock_bh() stop
> draining softirqs. The loop already runs under rcu_read_lock() and
> cannot sleep. What it gives up is preemption under CONFIG_PREEMPT,
> since a BH-off region is not preemptible outside PREEMPT_RT. That
> region is bounded by the skb and the table size, where the softirq
> backlog it used to absorb is not.

Sashiko argues that the table size is not really bounded, which is
fair, so it may be good to try and reword the argument a bit.  Maybe
point again to the fact that these buckets are empty and the walk
should be fast enough.  Not very important, only asking because there
is a couple of things to change below anyway.

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
> v2:
> - leave ovs_vport_cmd_dump() alone: nsid_lock has not been BH-safe
>   since commit aed4969f2bdf ("net: net->nsid_lock does not need BH
>   safety"), so the vport dump never drained softirqs
> - disable BH before the table dereference and say in a comment that
>   the region is not there for safety
> - drop the ovs_flow_stats_get() history, note the empty-bucket walk
>   and the lost CONFIG_PREEMPT preemption in the message
> - move the Cc list out of the commit message, add the net prefix

You say here that the net prefix was added, but it wasn't.  It should
be '[PATCH net v2]'.  But also, if you're targeting the net tree then,
as also noted by the sashiko, you need a Fixes tag (63e7959c4b9b would
work, I suppose) and the Cc for stable in the commit message tag section.

Also, please, add links to previous versions of the patch here in the
changelog section.  Especially if you're renaming the patch between
versions.

> 
>  net/openvswitch/datapath.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 631a03136fa1..a80bac81c043 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1532,6 +1532,11 @@ 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 here.

The word 'here' reads strange, I'd suggest removing it.

> +	 */
> +	local_bh_disable();

Since the comment only applies to the local_bh_disable(), I'd suggest
having an empty line here.

>  	ti = rcu_dereference(dp->table.ti);
>  	for (;;) {
>  		struct sw_flow *flow;
> @@ -1552,6 +1557,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  		cb->args[0] = bucket;
>  		cb->args[1] = obj;
>  	}

And an empty line here for symmetry.

> +	local_bh_enable();
>  	rcu_read_unlock();
>  	return skb->len;
>  }