Hello Jan,
Thank you for the review.
The recirc_id_node, and thus the frozen_state with the the ofproto_uuid can be
retrieved from recirc ID via recirc_id_node_find(). So I think, it would be
feasible to get the ofproto from the recirc ID without calling tnl_find(). In
addition we would need to store in_port in the frozen_state.
However, if we do not fix this in tnl_find(), then we need to make sure, that
neither xlate_lookup_ofproto() nor xlate_lookup() are called after
flow->packet_type has changed during xlate, don't we? For instance, calling
revalidate() can lead to call xlate_lookup().
Btw, what about the case, when we have a L2 and L3 port with the same local IP.
A packet is received on one of them, but the packet_type has changed during
xlate and later revalidate leads to calling xlate_lookup()? It will provide the
wrong tunnel port as in_port. Is this a valid case?
I was thinking about storing the original value of packet_type of received
packet in dp_packet or in the flow structure. Then use the original value in
tnl_find().
Best regards,
Zoltan
> -----Original Message-----
> From: Jan Scheurich
> Sent: Friday, December 08, 2017 11:30 AM
> To: Zoltán Balogh <zoltan.bal...@ericsson.com>; d...@openvswitch.org
> Cc: Ben Pfaff <b...@ovn.org>
> Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed
>
> Hi Zoltan,
>
> My feeling here is that it is conceptually wrong to do another tunnel lookup
> if the packet is recirculated after it
> has already been de-tunneled. You are trying to fix the symptoms and I am not
> sure that the more liberal check on
> packet type won't lead to incorrect proper tunnel port lookups.
>
> The actual bridge is stored in the frozen state of the recirculation context,
> and the OF in_port should be stored
> there as well (if it is not yet). I believe a recirculation should never
> change the bridge and the OF in_port. @Ben:
> Can you confirm?
>
> I think the order of things is wrong in an upcall. First ofproto should check
> if it is a recirculation upcall, and
> only if it is not, it should derive the bridge and the in_port from the flow.
> Today the first recirculation check
> happens later in upcall_xlate().
>
> Can you look into that option and propose a better way to solve the issue?
>
> Thanks, Jan
>
> > -----Original Message-----
> > From: Zoltán Balogh
> > Sent: Friday, 08 December, 2017 10:56
> > To: d...@openvswitch.org
> > Cc: Zoltán Balogh <zoltan.bal...@ericsson.com>; Jan Scheurich
> > <jan.scheur...@ericsson.com>; Ben Pfaff
> <b...@ovn.org>
> > Subject: [PATCH] tunnel: fix tnl_find() after packet_type changed
> >
> > During xlate, it can happen that tnl_find() is invoked when flow
> > packet_type has been already changed. For instance, pop_mpls and
> > resubmit actions should be applied to the packet in overlay bridge after
> > packet was received on a legacy_l3 tunnel port.
> > In this case, packet is recirculated after pop_mpls, a new tunnel lookup
> > is performed in order to find the proper ofproto, however packet_type of
> > flow is already PT_ETHERNET while the tunnel port mode is
> > NETDEV_PT_LEGACY_L3. So, no tunnel port is found and the packet is
> > dropped.
> >
> > This fix does an additional tnl_find() if no port is found. It looks for
> > L3 tunnel port in case of L2 packet and vice versa.
> >
> > Signed-off-by: Zoltan Balogh <zoltan.bal...@ericsson.com>
> > CC: Jan Scheurich <jan.scheur...@ericsson.com>
> > CC: Ben Pfaff <b...@ovn.org>
> > Fixes: 875ab13020b1 ("userspace: Handling of versatile tunnel ports")
> > ---
> > ofproto/tunnel.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> > index 1676f4d46..d497f026b 100644
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> > @@ -585,6 +585,19 @@ tnl_find(const struct flow *flow)
> > OVS_REQ_RDLOCK(rwlock)
> > if (tnl_port) {
> > return tnl_port;
> > }
> > +
> > + /* Maybe flow->packet_type has been already changed
> > during
> > + * xlate, so let's try to find L2 port for L3 packet or
> > + * L3 port for L2 packet. */
> > + if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE) {
> > + match.pt_mode = NETDEV_PT_LEGACY_L2;
> > + } else {
> > + match.pt_mode = NETDEV_PT_LEGACY_L3;
> > + }
> > + tnl_port = tnl_find_exact(&match, map);
> > + if (tnl_port) {
> > + return tnl_port;
> > + }
> > }
> >
> > i++;
> > --
> > 2.14.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
