The tc flow dump translates each tc flower rule into odp actions inside a fixed-size OFPBUF_STACK buffer of ODPUTIL_FLOW_KEY_BYTES. Unlike the key and the mask, the actions have no such bound: a single flower rule can carry up to TCA_ACT_MAX_PRIO actions, and a tunnel_push plus output pair alone takes about 100 bytes. When a rule does not fit, ofpbuf_resize__() hits OVS_NOT_REACHED() and ovs-vswitchd aborts.
This was hit in production with flood rules that output to many remote tunnel endpoints. Every non-terse "ovs-appctl dpctl/dump-flows" (every type= except "ovs") walks this path, so a monitoring agent polling the flows crash-looped ovs-vswitchd on each poll. Revalidators use terse dumps and are not affected. Use a stub buffer per batch slot instead, so the actions grow onto the heap when needed. The stub is set up once per dump thread and a grown slot keeps its heap buffer for the lifetime of the thread, which is safe because parse_tc_flower_to_match() clears the buffer before writing to it. tc_parse_flow_get() has the same problem with its 1024-byte buffer and is fixed the same way. The added test sends a flow to four Geneve tunnels with a 124-byte option each, which dumps back as about 750 bytes of odp actions, and dumps it with type=tc. Without the fix ovs-vswitchd aborts on the dump. CC: Paul Blakey <[email protected]> Fixes: f2280b419860 ("dpif-netlink: Dump netdevs flows on flow dump") Assisted-by: Claude Fable 5.1, Claude Code Signed-off-by: Loke Berne <[email protected]> --- v2: Resend only, v1 was mangled in transit and did not apply. No change to the patch itself. lib/dpif-offload-tc.c | 25 ++++++++++++--- tests/system-offloads-traffic.at | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c index 3b84d9f20..60822c1d8 100644 --- a/lib/dpif-offload-tc.c +++ b/lib/dpif-offload-tc.c @@ -62,7 +62,13 @@ struct tc_flow_dump_thread { struct ofpbuf nl_flows; struct odputil_keybuf keybuf[FLOW_DUMP_MAX_BATCH]; struct odputil_keybuf maskbuf[FLOW_DUMP_MAX_BATCH]; + /* Actions have no upper bound comparable to ODPUTIL_FLOW_KEY_BYTES: a + * single tc flower rule may carry up to TCA_ACT_MAX_PRIO actions, each + * translating into a tunnel push, a masked set, a ct or an output. The + * fixed-size 'actbuf' is therefore only a stub; 'act' grows onto the + * heap when a flow does not fit. */ struct odputil_keybuf actbuf[FLOW_DUMP_MAX_BATCH]; + struct ofpbuf act[FLOW_DUMP_MAX_BATCH]; }; static struct tc_offload * @@ -413,13 +419,13 @@ tc_flow_dump_next(struct dpif_offload_flow_dump_thread *thread_, while (!thread->netdev_dump_done && n_flows < max_flows) { struct odputil_keybuf *maskbuf = &thread->maskbuf[n_flows]; struct odputil_keybuf *keybuf = &thread->keybuf[n_flows]; - struct odputil_keybuf *actbuf = &thread->actbuf[n_flows]; struct netdev_tc_flow_dump *netdev_dump; struct dpif_flow *f = &flows[n_flows]; int cur = thread->netdev_dump_index; - struct ofpbuf key, mask, act; + struct ofpbuf *act = &thread->act[n_flows]; struct dpif_flow_stats stats; struct dpif_flow_attrs attrs; + struct ofpbuf key, mask; struct nlattr *actions; struct match match; ovs_u128 ufid; @@ -427,11 +433,10 @@ tc_flow_dump_next(struct dpif_offload_flow_dump_thread *thread_, netdev_dump = thread->dump->netdev_dumps[cur]; ofpbuf_use_stack(&key, keybuf, sizeof *keybuf); - ofpbuf_use_stack(&act, actbuf, sizeof *actbuf); ofpbuf_use_stack(&mask, maskbuf, sizeof *maskbuf); has_next = tc_netdev_flow_dump_next(netdev_dump, &match, &actions, &stats, &attrs, &ufid, - &thread->nl_flows, &act); + &thread->nl_flows, act); if (has_next) { tc_match_to_dpif_flow(&match, &key, &mask, actions, &stats, &attrs, &ufid, f, thread->dump->dump.terse); @@ -473,6 +478,10 @@ tc_flow_dump_thread_create(struct dpif_offload_flow_dump *dump) thread->netdev_dump_index = 0; thread->netdev_dump_done = !thread->dump->netdev_dump_count; ofpbuf_init(&thread->nl_flows, NL_DUMP_BUFSIZE); + for (int i = 0; i < FLOW_DUMP_MAX_BATCH; i++) { + ofpbuf_use_stub(&thread->act[i], &thread->actbuf[i], + sizeof thread->actbuf[i]); + } return &thread->thread; } @@ -481,6 +490,9 @@ tc_flow_dump_thread_destroy(struct dpif_offload_flow_dump_thread *thread_) { struct tc_flow_dump_thread *thread = tc_flow_dump_thread_cast(thread_); + for (int i = 0; i < FLOW_DUMP_MAX_BATCH; i++) { + ofpbuf_uninit(&thread->act[i]); + } ofpbuf_uninit(&thread->nl_flows); free(thread); } @@ -628,7 +640,8 @@ tc_parse_flow_get(struct tc_offload *offload_tc, struct dpif_flow_get *get) struct ofpbuf buf; int err = ENOENT; - ofpbuf_use_stack(&buf, &act_buf, sizeof act_buf); + /* Stub, not stack: the translated actions may exceed 'act_buf'. */ + ofpbuf_use_stub(&buf, &act_buf, sizeof act_buf); DPIF_OFFLOAD_PORT_FOR_EACH (port, &offload_tc->offload) { if (!tc_netdev_flow_get(port->netdev, &match, &actions, get->ufid, @@ -639,6 +652,7 @@ tc_parse_flow_get(struct tc_offload *offload_tc, struct dpif_flow_get *get) } if (err) { + ofpbuf_uninit(&buf); return err; } @@ -652,6 +666,7 @@ tc_parse_flow_get(struct tc_offload *offload_tc, struct dpif_flow_get *get) ofpbuf_put(get->buffer, nl_attr_get(actions), nl_attr_get_size(actions)); dpif_flow->actions = ofpbuf_at(get->buffer, 0, 0); dpif_flow->actions_len = nl_attr_get_size(actions); + ofpbuf_uninit(&buf); return 0; } diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index cfcc5bd6c..44f0c840d 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -1285,3 +1285,55 @@ m4_for([id], [2], [17], [1], [ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([offloads - dump flow with large actions]) +OVS_CHECK_GENEVE() +OVS_TRAFFIC_VSWITCHD_START([], [], + [-- set Open_vSwitch . other_config:hw-offload=true]) +AT_CHECK([ovs-appctl vlog/set tc:file:dbg dpif_offload_tc:file:dbg]) + +ADD_BR([br-underlay]) +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) + +ADD_NAMESPACES(at_ns0, at_ns1) + +dnl Underlay: the namespace side owns the four remote tunnel endpoints. +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") +m4_for([id], [2], [4], [1], [ + NS_CHECK_EXEC([at_ns0], [ip addr add 172.31.1.id/24 dev p0]) +]) +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) +AT_CHECK([ip link set dev br-underlay up]) + +m4_for([id], [1], [4], [1], [ + AT_CHECK([ovs-vsctl add-port br0 at_gnv[]id -- \ + set int at_gnv[]id type=geneve options:remote_ip=172.31.1.id \ + options:key=id]) +]) + +dnl Overlay source. Nothing answers for 10.1.1.2, so resolve it statically. +ADD_VETH(p1, at_ns1, br0, "10.1.1.1/24") +NS_CHECK_EXEC([at_ns1], [ip neigh replace 10.1.1.2 lladdr 02:00:00:00:00:02 dev p1]) + +dnl Send every packet from p1 to the four tunnels with a 124-byte Geneve +dnl option. That is only 8 tc actions (tunnel_key set + mirred per output), +dnl but each output dumps back as set(tunnel(...geneve(...))) + output, about +dnl 190 bytes, so the odp actions of the flow are over 750 bytes. The dump +dnl used to parse them into an ODPUTIL_FLOW_KEY_BYTES (640) sized stack +dnl buffer and abort on overflow. +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0,len=124}->tun_metadata0"]) +AT_CHECK([ovs-ofctl add-flow br0 "priority=10,in_port=ovs-p1,ip,actions=set_field:0x1234->tun_metadata0,output:at_gnv1,output:at_gnv2,output:at_gnv3,output:at_gnv4"]) + +dnl No reply is expected, the copies go out through the tunnels. +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -W 1 10.1.1.2], [ignore], [ignore]) +AT_CHECK([ovs-appctl revalidator/wait]) + +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc], [0], [stdout]) +AT_CHECK([grep "in_port(ovs-p1)" stdout | grep "eth_type(0x0800)" | head -1 | \ + grep -o "set[[(]]tunnel[[(]]" | wc -l], [0], [dnl +4 +]) +AT_CHECK([grep "in_port(ovs-p1)" stdout | grep -q "geneve[[(]][[{]]class=0xffff,type=0,len=124,"]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP -- 2.54.0 (Apple Git-157)
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
