| Message ID | 168416414918.7273.15419064875718310114.stgit@rawp |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: Fix recirculation with patch port and controller. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/15/23 17:22, 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, retrieves the xport starting > from the ofproto_uuid stored in the recirc node. > > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > ofproto/ofproto-dpif-xlate.c | 11 +++++++++-- > tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index c01177718..3509cc73c 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -1533,8 +1533,15 @@ 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) { > - struct uuid xport_uuid = recirc_id_node->state.xport_uuid; > - xport = xport_lookup_by_uuid(xcfg, &xport_uuid); > + if (uuid_is_zero(&recirc_id_node->state.xport_uuid)) { > + const struct xbridge *bridge = > + xbridge_lookup_by_uuid(xcfg, &recirc_id_node->state.ofproto_uuid); > + xport = bridge ? get_ofp_port(bridge, in_port) : NULL; IIUC, xport_uuid is designed to not be uuid of the patch port. But the in_port here is a patch port, right? So, we will find a different xport, right? Shouldn't we just fall into the else condition that handles NONE and CONTROLLER and not look for xport? Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 5/15/23 17:22, 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, retrieves the xport starting >> from the ofproto_uuid stored in the recirc node. >> >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> ofproto/ofproto-dpif-xlate.c | 11 +++++++++-- >> tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ >> 2 files changed, 43 insertions(+), 2 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index c01177718..3509cc73c 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -1533,8 +1533,15 @@ 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) { >> - struct uuid xport_uuid = recirc_id_node->state.xport_uuid; >> - xport = xport_lookup_by_uuid(xcfg, &xport_uuid); >> + if (uuid_is_zero(&recirc_id_node->state.xport_uuid)) { >> + const struct xbridge *bridge = >> + xbridge_lookup_by_uuid(xcfg, &recirc_id_node->state.ofproto_uuid); >> + xport = bridge ? get_ofp_port(bridge, in_port) : NULL; > > IIUC, xport_uuid is designed to not be uuid of the patch port. > But the in_port here is a patch port, right? So, we will find > a different xport, right? > > Shouldn't we just fall into the else condition that handles > NONE and CONTROLLER and not look for xport? > I guess it's ok to fall in the else in this case. The only problem is that we'd return the ofproto even if the in_port is invalid. This would make in turn fail "conntrack - fragment reassembly with L3 L4 protocol information". This test was fixed in the past after it already broke once 323ae1e808e6 ("ofproto-dpif-xlate: Fix recirculation when in_port is OFPP_CONTROLLER.") fixed the use case involving packet-out and recirculation. One possibility is to just retrieve the xport for that case in order to verify the in_port belongs to the bridge, without returning it (so honoring the xport_uuid logic). Maybe this could be done in the else branch so to make clear we're handling the special case related to OFPP_{NONE,CONTROLLER}. WDYT? > Best regards, Ilya Maximets.
On 5/23/23 00:16, Paolo Valerio wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> On 5/15/23 17:22, 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, retrieves the xport starting >>> from the ofproto_uuid stored in the recirc node. >>> >>> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 11 +++++++++-- >>> tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ >>> 2 files changed, 43 insertions(+), 2 deletions(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index c01177718..3509cc73c 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -1533,8 +1533,15 @@ 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) { >>> - struct uuid xport_uuid = recirc_id_node->state.xport_uuid; >>> - xport = xport_lookup_by_uuid(xcfg, &xport_uuid); >>> + if (uuid_is_zero(&recirc_id_node->state.xport_uuid)) { >>> + const struct xbridge *bridge = >>> + xbridge_lookup_by_uuid(xcfg, &recirc_id_node->state.ofproto_uuid); >>> + xport = bridge ? get_ofp_port(bridge, in_port) : NULL; >> >> IIUC, xport_uuid is designed to not be uuid of the patch port. >> But the in_port here is a patch port, right? So, we will find >> a different xport, right? >> >> Shouldn't we just fall into the else condition that handles >> NONE and CONTROLLER and not look for xport? >> > > I guess it's ok to fall in the else in this case. > The only problem is that we'd return the ofproto even if the in_port is > invalid. > This would make in turn fail "conntrack - fragment reassembly with L3 L4 > protocol information". This test was fixed in the past after it already > broke once 323ae1e808e6 ("ofproto-dpif-xlate: Fix recirculation when > in_port is OFPP_CONTROLLER.") fixed the use case involving packet-out > and recirculation. > > One possibility is to just retrieve the xport for that case in order to > verify the in_port belongs to the bridge, without returning it (so > honoring the xport_uuid logic). Maybe this could be done in the else > branch so to make clear we're handling the special case related to > OFPP_{NONE,CONTROLLER}. > > WDYT? It seems like there should be a better generic solution for all this, but sounds OK. > >> Best regards, Ilya Maximets. >
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index c01177718..3509cc73c 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1533,8 +1533,15 @@ 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) { - struct uuid xport_uuid = recirc_id_node->state.xport_uuid; - xport = xport_lookup_by_uuid(xcfg, &xport_uuid); + if (uuid_is_zero(&recirc_id_node->state.xport_uuid)) { + const struct xbridge *bridge = + xbridge_lookup_by_uuid(xcfg, &recirc_id_node->state.ofproto_uuid); + xport = bridge ? get_ofp_port(bridge, in_port) : NULL; + } else { + 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) { goto out; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 6824ce0bb..8b9447c74 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
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, retrieves the xport starting from the ofproto_uuid stored in the recirc node. Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 11 +++++++++-- tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-)