On 5/23/23 00:16, Paolo Valerio wrote: > Ilya Maximets <i.maxim...@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 <pvale...@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. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev