diff mbox series

[ovs-dev,v2] ofproto-dpif-xlate: Fix recirculation with patch port and controller.

Message ID 169394118209.2946148.11773928893766161946.stgit@rawp
State Accepted
Commit 154e4299de6a6fb0ac20863fc7a062e7348044c1
Headers show
Series [ovs-dev,v2] ofproto-dpif-xlate: Fix recirculation with patch port and controller. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Paolo Valerio Sept. 5, 2023, 7:13 p.m. UTC
If a packet originating from the controller recirculates after going
through a patch port, it gets dropped with the following message:

ofproto_dpif_upcall(handler8)|INFO|received packet on unassociated
  datapath port 4294967295

This happens because there's no xport_uuid in the recirculation node
and at the same type in_port refers to the patch port.

The patch, in the case of zeroed uuid, checks that in_port belongs to
the bridge and returns the related ofproto.

Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
 ofproto/ofproto-dpif-xlate.c |   12 +++++++++++-
 tests/ofproto-dpif.at        |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Sept. 6, 2023, 11:13 p.m. UTC | #1
On 9/5/23 21:13, Paolo Valerio wrote:
> If a packet originating from the controller recirculates after going
> through a patch port, it gets dropped with the following message:
> 
> ofproto_dpif_upcall(handler8)|INFO|received packet on unassociated
>   datapath port 4294967295
> 
> This happens because there's no xport_uuid in the recirculation node
> and at the same type in_port refers to the patch port.
> 
> The patch, in the case of zeroed uuid, checks that in_port belongs to
> the bridge and returns the related ofproto.
> 
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
> ---
>  ofproto/ofproto-dpif-xlate.c |   12 +++++++++++-
>  tests/ofproto-dpif.at        |   34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 47ea0f47e..fcd547645 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1615,7 +1615,8 @@  xlate_lookup_ofproto_(const struct dpif_backer *backer,
         }
 
         ofp_port_t in_port = recirc_id_node->state.metadata.in_port;
-        if (in_port != OFPP_NONE && in_port != OFPP_CONTROLLER) {
+        if (in_port != OFPP_NONE && in_port != OFPP_CONTROLLER &&
+            !uuid_is_zero(&recirc_id_node->state.xport_uuid)) {
             struct uuid xport_uuid = recirc_id_node->state.xport_uuid;
             xport = xport_lookup_by_uuid(xcfg, &xport_uuid);
             if (xport && xport->xbridge && xport->xbridge->ofproto) {
@@ -1626,11 +1627,19 @@  xlate_lookup_ofproto_(const struct dpif_backer *backer,
              * that the packet originated from the controller via an OpenFlow
              * "packet-out".  The right thing to do is to find just the
              * ofproto.  There is no xport, which is OK.
+             * Also a zeroed xport_uuid with a valid in_port, means that
+             * the packet originated from OFPP_CONTROLLER passed
+             * through a patch port.
              *
              * OFPP_NONE can also indicate that a bond caused recirculation. */
             struct uuid uuid = recirc_id_node->state.ofproto_uuid;
             const struct xbridge *bridge = xbridge_lookup_by_uuid(xcfg, &uuid);
+
             if (bridge && bridge->ofproto) {
+                if (in_port != OFPP_CONTROLLER && in_port != OFPP_NONE &&
+                    !get_ofp_port(bridge, in_port)) {
+                    goto xport_lookup;
+                }
                 if (errorp) {
                     *errorp = NULL;
                 }
@@ -1643,6 +1652,7 @@  xlate_lookup_ofproto_(const struct dpif_backer *backer,
         }
     }
 
+xport_lookup:
     xport = xport_lookup(xcfg, tnl_port_should_receive(flow)
                          ? tnl_port_receive(flow)
                          : odp_port_to_ofport(backer, flow->in_port.odp_port));
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index f242f77f3..a0a4aaf5d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5854,6 +5854,40 @@  OVS_WAIT_UNTIL([check_flows], [ovs-ofctl dump-flows br0])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# Checks for regression against a bug in which OVS dropped packets
+# originating from the the controller passing through a patch port
+AT_SETUP([ofproto-dpif - packet-out recirculation OFPP_CONTROLLER and patch port])
+OVS_VSWITCHD_START(
+    [add-port br0 patch-br1 -- \
+     set interface patch-br1 type=patch options:peer=patch-br0 -- \
+     add-br br1 -- set bridge br1 datapath-type=dummy fail-mode=secure -- \
+     add-port br1 patch-br0 -- set interface patch-br0 type=patch options:peer=patch-br1
+])
+
+add_of_ports --pcap br1 1
+
+AT_DATA([flows-br0.txt], [dnl
+table=0 icmp actions=output:patch-br1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows-br0.txt])
+
+AT_DATA([flows-br1.txt], [dnl
+table=0, icmp actions=ct(table=1,zone=1)
+table=1, ct_state=+trk, icmp actions=p1
+])
+AT_CHECK([ovs-ofctl add-flows br1 flows-br1.txt])
+
+packet=50540000000750540000000508004500005c000000008001b94dc0a80001c0a80002080013fc00000000000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f
+AT_CHECK([ovs-ofctl packet-out br0 "in_port=CONTROLLER packet=$packet actions=table"])
+
+OVS_WAIT_UNTIL_EQUAL([ovs-ofctl dump-flows -m br1 | grep "ct_state" | ofctl_strip], [dnl
+ table=1, n_packets=1, n_bytes=106, ct_state=+trk,icmp actions=output:2])
+
+OVS_WAIT_UNTIL([ovs-pcap p1-tx.pcap | grep -q "$packet"])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - debug_slow action])
 OVS_VSWITCHD_START
 add_of_ports br0 1 2 3