On 10/2/26 12:27 AM, [email protected] 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
