Hello Jan,
> 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_():
I know, that ofproto_uuid (bridge to resume from) and in_port (incoming port)
are
stored in frozen_state and frozen_metadata. As I mentioned these in a private
message yesterday. I also wrote to you, that the stored ofproto_uuid refers to
the
bridge to resume from, and this can differ from the one xlate_lookup_ofproto_()
could provide. For instance, in the case of using patch ports and recirculate
in
the second bridge. Please, double check your mail-box.
>
> 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.
>
Actually, because of xlate_lookup_ofproto_() could provide a different ofproto,
than stored in frozen_state, we cannot use ofproto_uuid from frozen_state to
retrieve the correct xport. Am I wrong?
> Please also consider and deal with the other bug we found in xlate_actions:
>
I hope, Ben (or the competent maintainer) will answer my question in my previous
e-mail and clarify if the code you mentioned below needs to be modified or not.
Best regards,
Zoltan
> 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
