Hi Zoltan, As far as I can see the frozen state already contains the ofproto UUID and in frozen_metadata the OF port ID. Together these should be sufficient to look up the xport in xlate_lookup_ofproto_():
First look up the xbridge through xbridge_lookup_by_uuid() and afterwards scan the xports hmap of the xbridge for the OF port ID as done in static function get_ofp_port(xbridge, ofp_port). Thus, there should be no need to add a UUID to the xport and store that in frozen_state. Please also consider and deal with the other bug we found in xlate_actions: diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index d94e9dc..bfa3acd 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7037,11 +7037,28 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) } ctx.wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; + /* FIXME: The line below looks up the ingress xport from the in_port + * in base_flow, which is populated with the initial OF port + * determined early in the upcall from the ODP port in the dp_packet's + * flow. In the case of recirculation in a subsequent bridge (think patch + * port or tunnel port) the ODP port is a port in the initial bridge, + * and its OF port number has no meaning in the current bridge. In the + * best case there is a miss, in the worst case the base_flow.in_port + * matches a bogus port in the current bridge. */ + /* Get the proximate input port of the packet. (If xin->frozen_state, * flow->in_port is the ultimate input port of the packet.) */ struct xport *in_port = get_ofp_port(xbridge, ctx.base_flow.in_port.ofp_port); + /* It would be more correct to look up the xport from + * ctx.in.flow.in_port, which after recirculation has already been set + * with the thawed in_port in frozen_metadata_to_flow() above. + * Only in the case of bond recirculation there will be no valid in_port + * in the static recirculation context. But perhaps this is not a real + * problem as the output to bond action is typically the last action + * and the in_port won't matter anymore. */ + if (flow->packet_type != htonl(PT_ETH) && in_port && in_port->pt_mode == NETDEV_PT_LEGACY_L3 && ctx.table_id == 0) { /* Add dummy Ethernet header to non-L2 packet if it's coming from a BR, Jan > -----Original Message----- > From: ovs-dev-boun...@openvswitch.org > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Zoltán Balogh > Sent: Friday, 05 January, 2018 12:27 > To: Ben Pfaff <b...@ovn.org> > Cc: 'd...@openvswitch.org' <d...@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH] xlate: fix xport lookup for recirc > > > On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote: > > > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto > > > based on xport determined using flow, which is extracted from packet. > > > The lookup can happen due to recirculation as well. It can happen, that > > > packet_type has been modified during xlate before recirculation is > > > triggered, so the lookup fails or delivers wrong xport. > > > This can be worked around by propagating xport to ctx->xin after the very > > > first lookup and store it in frozen state of the recirculation. > > > So, when lookup is performed due to recirculation, the xport can be > > > retrieved from the frozen state. > > > > > > Signed-off-by: Zoltan Balogh <zoltan.bal...@ericsson.com> > > > CC: Jan Scheurich <jan.scheur...@ericsson.com> > > > > Thanks for working on finding and fixing bugs. > > > > Storing a pointer to an xport, then checking later that it points to a > > valid xport, is risky because it opens up the possibility that the > > pointer points to a different xport that just happens to have the same > > address. It's hard to guess how likely that coincidence is, but it > > would be better to avoid it. This is the reason that frozen_state uses > > a random UUID to locate "ofprotos". Probably, that approach would be > > better for xports too. > > I see, thank you for the explanation. I'm going to create a new patch using > xport UUIDs. > > > > > When xport_in is nonnull but invalid, wouldn't it be better to abort > > than to search for an xport the fallback way? > > Yes, that would be better. Especially if UUID is used to get the xport. > > > > > What is the reason for the special case for patch ports? > > I've performed some tests before created the patch. I had a setup with two > bridges connected with patch ports: > > p1 +-------+ +-------+ p2 > ->-o | | o->- > | br1 o-----o br2 | > +-------+ +-------+ > > Recirculation can occur in the 2nd bridge as well, for instance when pop_mpls > action is followed by resubmit. In this case a new upcall is performed and > xlate_lookup_ofproto_() and xport_lookup() are invoked again. As I observed, > xport_lookup() returns xport referring to p1 in br1, i.e. the very first port > where the packet was received on. I assume, this is not a bug in OVS, is it? > > So, I would like to store this very first port in the frozen state. That's the > reason why I do store xport in ctx.xin->xport_in only if xport is not a patch > port. If it's not a patch port, it can be only the very first input port. > > > > > Can you provide a good example of where this is important? > > The main goal of this patch is to avoid invocation of xlate_lookup() in case > of > recirculation (except recirc due to bond), because it can return a wrong > xport. > For instance, if L3 packet with MPLS label is received on a L3 tunnel port and > pop_mpls + resubmit actions are performed, then first packet_type is changed > due to pushing a dummy ethernet header, MPLS label is removed, then resubmit > action is processed. This triggers recirculation, where xport_lookup() fails > due to former change of packet_type as I noted in the commit message. > > > > > Thanks, > > > > Ben. > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev