On 9/2/26 10:08 AM, Salem Sol 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]>
> ---
> ofproto/ofproto-dpif-xlate.c | 23 ++++++++++++++++++++++-
> tests/ofproto-dpif.at | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 764dbd6627..3a64fc9ec6 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1690,7 +1690,10 @@ 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 OpenFlow action (e.g.
> load:0xffff->NXM_OF_IN_PORT[])
> + * cleared the OF in_port mid-pipeline before a freeze (as OVN
> + * does at logical-datapath crossings). */
> struct uuid uuid = recirc_id_node->state.ofproto_uuid;
> const struct xbridge *bridge = xbridge_lookup_by_uuid(xcfg,
> &uuid);
>
> @@ -1699,6 +1702,24 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer,
> !get_ofp_port(bridge, in_port)) {
> goto xport_lookup;
> }
> +
> + /* If there is a datapath port associated with this flow and
> + * it no longer exists, the flow is stale and must be
> + * evicted, so fall through to xport_lookup. ODPP_NONE means
> + * there is no datapath port at all (e.g. a controller
> + * packet-out), so skip the check in that case. */
This comment is still way too long. The whole second part of it is
unnecessary as is it already implied by the "If there is a datapath
port associated", which perfectly explains the 'if' condition.
> + 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 ee6ac873d8..2bcefbc93b 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6646,6 +6646,41 @@ OVS_WAIT_UNTIL([check_flows], [ovs-ofctl dump-flows
> br0])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - evict recirculated OFPP_NONE flows on input port
> removal])
> +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])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> dnl Checks for regression against a bug in which OVS dropped packets
> dnl originating from a controller passing through a patch port.
> AT_SETUP([ofproto-dpif - packet-out recirculation OFPP_CONTROLLER and patch
> port])
I was asking to put the new test after this one, not before. Placing in
between of two 'ofproto-dpif - packet-out' tests doesn't make much sense.
To avoid further reporting of this patch, I cut and re-wrapped the comment,
moved the test and applied the change. Backported down to 3.3.
Thanks!
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
