Hi Ilya, thanks for your review. Please see updated patch in V5. Please ignore V4.
Thanks, > On 26 Aug 2026, at 0:49, Ilya Maximets <[email protected]> wrote: > > On 6/14/26 10:58 AM, Salem Sol via dev wrote: >> xlate_lookup_ofproto_() takes a shortcut when the frozen state has >> in_port == OFPP_NONE: it returns the bridge's ofproto via uuid lookup >> without further validation. The cited commit introduced this for >> controller-originated packet-outs, where there is no real ingress port. >> >> However, OVN also produces OFPP_NONE in the frozen state at logical >> datapath crossings by explicitly clearing in_port mid-pipeline >> (load:0xffff->in_port) before a ct() freeze. In this case the datapath >> flow still has a real in_port. >> >> When that port is deleted, the revalidator re-translates these flows. >> The shortcut bypasses port validation, the actions look unchanged, and >> the flows are kept. They then accumulate until max-idle fires, which is >> problematic in high-churn environments like Kubernetes, and especially >> in offload setups where matching flows are held in hardware. >> >> Fix this by verifying that the datapath flow's in_port still maps to an >> OF port via odp_port_to_ofport() in the shortcut path. Skip the check >> when in_port.odp_port == ODPP_NONE so the original controller packet-out >> scenarios continue to work. >> >> A regression test reproducing the bug on the dummy datapath is added. >> >> Fixes: 323ae1e808e6 ("ofproto-dpif-xlate: Fix recirculation when in_port is >> OFPP_CONTROLLER.") >> Assisted-by: Claude Opus 4.8, Cursor >> Signed-off-by: Salem Sol <[email protected]> >> --- > > Hi, Salem. Thanks for the update and sorry for delay. > See some comments below. > >> ofproto/ofproto-dpif-xlate.c | 22 +++++++++++++++++++- >> tests/ofproto-dpif.at | 39 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index c1ba447e93..d33b4afabf 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -1689,7 +1689,9 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer, >> * the packet originated from OFPP_CONTROLLER passed >> * through a patch port. >> * >> - * OFPP_NONE can also indicate that a bond caused >> recirculation. */ >> + * OFPP_NONE can also indicate that a bond caused recirculation, >> + * or that an action cleared in_port before a freeze (e.g. OVN >> at >> + * logical datapath crossings). */ >> struct uuid uuid = recirc_id_node->state.ofproto_uuid; >> const struct xbridge *bridge = xbridge_lookup_by_uuid(xcfg, >> &uuid); >> >> @@ -1698,6 +1700,24 @@ xlate_lookup_ofproto_(const struct dpif_backer >> *backer, >> !get_ofp_port(bridge, in_port)) { >> goto xport_lookup; >> } >> + >> + /* The OF in_port is OFPP_NONE/OFPP_CONTROLLER, but the >> + * datapath flow still ingresses on a concrete odp_port. >> + * If that port is gone the flow is dead, so fall through >> + * to xport_lookup and fail. ODPP_NONE has no datapath >> + * ingress (packet-out); skip it. */ > > This comment is now misleading. The in_port doesn't have to be > OFPP_NONE or OFPP_CONTROLLER. It can be a real port. The previous > comment above in the code explains a few other cases how we can > get here with a real OF port number, e.g. a patch port number. > > It may be enough to just say that if there is a datapath port > associated with this flow and it no longer exists, then the flow > is stale. > >> + if (flow->in_port.odp_port != ODPP_NONE) { >> + const struct ofport_dpif *ingress; >> + >> + ingress = tnl_port_should_receive(flow) >> + ? tnl_port_receive(flow) >> + : odp_port_to_ofport(backer, >> + flow->in_port.odp_port); >> + if (!ingress) { >> + goto xport_lookup; >> + } >> + } >> + >> if (errorp) { >> *errorp = NULL; >> } >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index dcab7bba45..cb4cc46a69 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -9010,6 +9010,45 @@ packets:2, bytes:68, used:0.001s, actions:drop >> OVS_VSWITCHD_STOP(["/sending to collector failed/d"]) >> AT_CLEANUP >> >> +dnl OVN clears the OF in_port (load:0xffff->NXM_OF_IN_PORT[]) before a ct() >> +dnl freeze, so the frozen state has in_port == OFPP_NONE while the datapath >> +dnl flow still keys on the real ingress port. Removing that port must evict >> +dnl the recirculated flow instead of keeping it until max-idle. >> +AT_SETUP([ofproto-dpif - evict recirc flows on in_port removal]) > > The test is placed in the middle of IPFIX tests. It should be moved to > a more appropriate location, e.g. after the "ofproto-dpif - packet-out > recirculation with OFPP_NONE and OFPP_CONTROLLER", that tests a similar > functionality. > > And it's better to use full words, e.g. "evict recirculated OFPP_NONE > flows on input port removal". This may also eliminate the need for the > comment above the test. > >> +AT_KEYWORDS([recirc revalidator]) >> +OVS_VSWITCHD_START >> +add_of_ports br0 1 2 >> + >> +m4_define([PKT], [m4_join([,], >> + [in_port(1)], >> + [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a)], >> + [eth_type(0x0800)], >> + [ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no)])]) >> + >> +AT_DATA([flows.txt], [dnl >> +table=0,in_port=1,tcp >> actions=load:0xffff->NXM_OF_IN_PORT[[]],ct(zone=1,table=1) >> +table=1,tcp actions=output:2 >> +]) >> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> + >> +dnl A few packets so both datapath flows get hits. >> +for i in 1 2 3; do >> + AT_CHECK([ovs-appctl netdev-dummy/receive p1 'PKT']) >> +done >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | strip_used | strip_stats | sort], >> [0], [dnl >> +flow-dump from the main thread: >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no), >> packets:0, bytes:0, used:0.0s, actions:ct(zone=1),recirc(0x1) >> +recirc_id(0x1),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no), >> packets:0, bytes:0, used:0.0s, actions:2 >> +]) >> + >> +AT_CHECK([ovs-vsctl del-port br0 p1]) >> +AT_CHECK([ovs-appctl revalidator/wait]) >> +AT_CHECK([ovs-appctl dpctl/dump-flows | strip_used | strip_stats | sort], >> [0], []) > > Should be just: > > AT_CHECK([ovs-appctl dpctl/dump-flows]) > > Since no output is expected. > >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> dnl Bridge IPFIX statistics check >> AT_SETUP([ofproto-dpif - Bridge IPFIX statistics check]) >> OVS_VSWITCHD_START > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
