tc_parse_flow_get() translates the tc rule into 'key' and 'mask' buffers on its own stack frame and returns a dpif_flow whose key and mask point into them. Only the actions were copied into the caller's buffer. The caller, for instance "ovs-appctl dpctl/get-flow", formats the flow after the function has returned and so reads a stale stack frame. It mostly works because nothing has reused that part of the stack yet.
Copy the key and the mask into the caller's buffer as well, and resolve all three pointers only after the last put, since any put may reallocate the buffer. The unused 'act' buffer goes away with it. The added test fetches every tc flow of a bridge by ufid and expects the same output as the flow dump gives. It passes on the old code too unless the stack happens to be reused in between, so it covers the get path rather than reproducing the bug. CC: Paul Blakey <[email protected]> Fixes: 6c343984809c ("dpif-netlink: Use netdev flow get api to query a flow") Assisted-by: Claude Fable 5.1, Claude Code Signed-off-by: Loke Berne <[email protected]> --- lib/dpif-offload-tc.c | 24 +++++++++++++++++++----- tests/system-offloads-traffic.at | 28 ++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c index 60822c1d8..91c5df9c8 100644 --- a/lib/dpif-offload-tc.c +++ b/lib/dpif-offload-tc.c @@ -630,12 +630,12 @@ tc_parse_flow_get(struct tc_offload *offload_tc, struct dpif_flow_get *get) struct dpif_offload_port *port; struct odputil_keybuf maskbuf; struct odputil_keybuf keybuf; - struct odputil_keybuf actbuf; - struct ofpbuf key, mask, act; + size_t key_ofs, mask_ofs, act_ofs; struct dpif_flow_stats stats; struct dpif_flow_attrs attrs; uint64_t act_buf[1024 / 8]; struct nlattr *actions; + struct ofpbuf key, mask; struct match match; struct ofpbuf buf; int err = ENOENT; @@ -659,13 +659,27 @@ tc_parse_flow_get(struct tc_offload *offload_tc, struct dpif_flow_get *get) VLOG_DBG("found flow from netdev, translating to dpif flow"); ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); - ofpbuf_use_stack(&act, &actbuf, sizeof actbuf); ofpbuf_use_stack(&mask, &maskbuf, sizeof maskbuf); tc_match_to_dpif_flow(&match, &key, &mask, actions, &stats, &attrs, (ovs_u128 *) get->ufid, dpif_flow, false); + + /* 'key', 'mask' and 'actions' live on this stack frame, so copy them + * into the caller's buffer and point the flow at the copies. Resolve + * the pointers only after the last put, as any put may reallocate the + * buffer. */ + key_ofs = get->buffer->size; + ofpbuf_put(get->buffer, dpif_flow->key, dpif_flow->key_len); + mask_ofs = get->buffer->size; + ofpbuf_put(get->buffer, dpif_flow->mask, dpif_flow->mask_len); + act_ofs = get->buffer->size; ofpbuf_pu t(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); + + dpif_flow->key = ofpbuf_at_assert(get->buffer, key_ofs, + dpif_flow->key_len); + dpif_flow->mask = ofpbuf_at_assert(get->buffer, mask_ofs, + dpif_flow->mask_len); + dpif_flow->actions = ofpbuf_at_assert(get->buffer, act_ofs, + dpif_flow->actions_len); ofpbuf_uninit(&buf); return 0; diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 44f0c840d..980465406 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -1337,3 +1337,31 @@ AT_CHECK([grep "in_port(ovs-p1)" stdout | grep -q "geneve[[(]][[{]]class=0xffff, OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([offloads - get flow by uf id]) +OVS_TRAFFIC_VSWITCHD_START([], [], + [-- set Open_vSwitch . other_config:hw-offload=true]) + +ADD_NAMESPACES(at_ns0, at_ns1) +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) + +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.1.1.2 | FORMAT_PING], [0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) +AT_CHECK([ovs-appctl revalidator/wait]) + +dnl Fetching a tc flow by ufid must give the same key, mask, actions and +dnl attributes as the dump does. Only the 'used' age may differ. +AT_CHECK([ovs-appctl dpctl/dump-flows -m type=tc | grep "eth_type(0x0800)" \ + | sed 's/used:[[0-9.]]*s/used:X/' | sort > dump.txt]) +AT_CHECK([test -s dump.txt]) +AT_CHECK([for ufid in $(sed -n 's/^\(ufid:[[-0-9a-f]]*\).*/\1/p' dump.txt); do + ovs-appctl dpctl/get-flow -m $ufid + done | sed 's/used:[[0-9.]]*s/used:X/' | sort > get.txt]) +AT_CHECK([diff dump.txt get.txt]) + +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
