[ovs-dev] [PATCH] tunnel: fix tnl_find() after packet_type changed
Zoltán Balogh
zoltan.balogh at ericsson.com
Mon Dec 11 15:38:39 UTC 2017
Hi,
I've been working on the solution proposed by Jan. I faced a new issue with
patch ports. Let's have a config like in unit test 1025:
"1025: ofproto-dpif - balance-tcp bonding, different recirc flow "
+-------------+
| br-int | p5 (OF 5)
| o--<---
+---o---------+
| br1- (patch port: OF 101)
|
+------+
|
| br1+ (patch port: OF 100)
+----------o--+
| br1 |
| |
+---o-----o---+
p3 | | p4
(OF 3)| | (OF 4)
|bond1|
+--+--+
|
In the unit test, a packet is received on port p5. The packet is then output to
'br1-' which is a patch port, its pair is 'br1+' on br1. Ports p3 and p4 are in
bond0, the packet is recirculated in br1, before it is sent out on bond1.
So, when it's recirculated, ofproto xlate will end up in xlate_lookup_ofproto_()
at some point in time. The vanilla code does xport_lookup() based on the flow
which is built up based on dp_packet (due to miniflow_extract/expand) which has
in port 5 (p5). So the flow has in port 5 as well. Furthermore, the returned
ofproto_dpif will be xport->xbridge->ofproto: br-int.
If we do use the recirc ID extracted from the flow (or from dp_packet) which is
2, then the state's uuid will refer to the bridge br1. This is the bridge where
the recirc action was added to odp_actions. On the other hand the in port
stored in the state is 0xffff (undefined). It's neither 5 (p5) nor 100 (br1+).
So, If we want to build our code based on the recirc ID extracted from flow (or
dp_packet) we won't get the same result as the vanilla code in case of using
patch ports and recirculate in the second bridge.
Does anyone have a idea how to resolve this?
Maybe the vanilla code or patch port concept is broken?
If we would use the original value of packet_type in tnl_find() we could still
workaround this.
Best regards,
Zoltan
> -----Original Message-----
> From: Jan Scheurich
> Sent: Friday, December 08, 2017 5:18 PM
> To: Zoltán Balogh <zoltan.balogh at ericsson.com>; dev at openvswitch.org
> Cc: Ben Pfaff <blp at ovn.org>
> Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed
>
> Hi Zoltan,
> Please find answers below.
> Regards, Jan
>
> > -----Original Message-----
> > From: Zoltán Balogh
> > Sent: Friday, 08 December, 2017 12:56
> > To: Jan Scheurich <jan.scheurich at ericsson.com>; dev at openvswitch.org
> > Cc: Ben Pfaff <blp at ovn.org>
> > Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed
> >
> > 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.
>
> Yes, my thinking was to lookup the recirculation context in xlate_lookup_ofproto_() if flow->recirc_id != 0 and get
> bridge and in_port from the frozen data in that case. But I was looking for a way to store the recirc context
> pointer so that it need not be looked up once more later during xlate_actions().
>
> BTW: in_port is already stored in frozen_state.metdata.in_port
>
> >
> > 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().
>
> revalidate() is called from revalidator threads to pass an artificial packet representing the megaflow through the
> pipeline. The flow constructed from the ukey will contain the recirc_id and it should still hit the same
> recirculation context with the frozen_state as a MISS upcall for the originally recirculated packet.
>
> >
> > 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 think xlate_lookup() is only executed once at the beginning of each upcall or ukey revalidation to initiatilize
> some data. It is never done again later during the xlation process.
>
> >
> > 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.balogh at ericsson.com>; dev at openvswitch.org
> > > Cc: Ben Pfaff <blp at 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: dev at openvswitch.org
> > > > Cc: Zoltán Balogh <zoltan.balogh at ericsson.com>; Jan Scheurich <jan.scheurich at ericsson.com>; Ben Pfaff
> > > <blp at 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.balogh at ericsson.com>
> > > > CC: Jan Scheurich <jan.scheurich at ericsson.com>
> > > > CC: Ben Pfaff <blp at 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
More information about the dev
mailing list