[ovs-dev] [PATCH] tunnel: fix tnl_find() after packet_type changed

[Jan Scheurich]

At first glance I would try to skip or enhance the port and bridge lookup in upcall_receive() or xlate_lookup() if flow->recirc_id != 0.

The code in xlate_actions() actually already today restores bridge and flow.in_port from the thawed frozen_state. We need to make sure that prior to reaching that point code does not stop working if the upcall bridge and in_port are not populated properly.

If there is code that requires this, we may want to move the lookup of the recirc node and frozen state a little bit up the call stack so that we can initialize it from the frozen state.

I wonder if recirc_id and frozen bridge and in_port are only relevant in the context of MISS upcalls or also for SFLOW, IPFIX and FLOW_SAMPLE upcalls. There seems to be some "magic" also in those parts of the code that guess the in_port for ingress tunneling, so they might also benefit.

Regards,  Jan

> -----Original Message-----
> From: Jan Scheurich
> Sent: Friday, 08 December, 2017 11:30
> 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