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
