[ovs-dev] [PATCH] xlate: fix xport lookup for recirc

Jan Scheurich jan.scheurich at ericsson.com
Fri Jan 5 13:18:54 UTC 2018


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-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Zoltán Balogh
> Sent: Friday, 05 January, 2018 12:27
> To: Ben Pfaff <blp at ovn.org>
> Cc: 'dev at openvswitch.org' <dev at 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.balogh at ericsson.com>
> > > CC: Jan Scheurich <jan.scheurich at 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list