[Devel] [PATCH vz10-latest 1/1] openvswitch: fix soft lockup in the netlink dump callbacks #VSTOR-144650

[Denis V. Lunev]

A production compute node carrying a few thousand datapath flows and
ports 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. ovs_vport_cmd_dump() calls
peernet2id_alloc() for every vport that lives in a foreign netns, and
that releases nsid_lock with spin_unlock_bh(). 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 items 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. Both loops already run under rcu_read_lock() and
cannot sleep. What they give 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.

The vport half is downstream-only. Mainline made nsid_lock a plain
spinlock in commit aed4969f2bdf ("net: net->nsid_lock does not need
BH safety"), so the vport dump there never drains softirqs and the
upstream submission touches ovs_flow_cmd_dump() alone. This kernel
predates that commit and still takes nsid_lock with spin_lock_bh().

Signed-off-by: Denis V. Lunev <den at openvz.org>
---
 net/openvswitch/datapath.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index f2f50324424b..762bd6d8e49f 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1478,6 +1478,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;
@@ -1498,6 +1503,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;
 }
@@ -2502,6 +2508,11 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		rcu_read_unlock();
 		return -ENODEV;
 	}
+	/*
+	 * Not needed for safety. Stops every spin_unlock_bh() in
+	 * peernet2id_alloc() from running the softirq backlog here.
+	 */
+	local_bh_disable();
 	for (i = bucket; i < DP_VPORT_HASH_BUCKETS; i++) {
 		struct vport *vport;
 
@@ -2522,6 +2533,7 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 		skip = 0;
 	}
 out:
+	local_bh_enable();
 	rcu_read_unlock();
 
 	cb->args[0] = i;
-- 
2.53.0