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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
