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
