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. 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.
The commit message says:
The region stays short: the skb caps the flows one callback emits, and
empty buckets cost no skb space but are each visited once per dump, as
the cursor only moves forward. The table grows on insert and shrinks
only on flush, so the walk is bounded by the largest flow count the
datapath has held.
That bound is real, but a namespace user chooses it. Is "stays short"
accurate here?
Two ways to keep the soft lockup fix without an unbounded BH-off window:
- Stop after a fixed number of buckets per callback and resume from
cb->args.
- Re-enable and disable BH after each batch of buckets.
Would either of these work?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929072519.2803304-2-den%40openvz.org
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
