Re: [dpdk-dev] [PATCH] net/mlx5: fix GRE flow rule

[Matan Azrad <matan@mellanox.com>]

Hi Yongseok

From: Yongseok Koh
> On Wed, May 23, 2018 at 04:45:33AM -0700, Matan Azrad wrote:
> >
> > Hi Yongseok
> >  + Steven
> >
> >  From: Yongseok Koh
> > > On Tue, May 22, 2018 at 10:36:43PM -0700, Matan Azrad wrote:
> > > > Hi Yongseok
> > > >
> > > > From:  Yongseok Koh
> > > > > +#ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT
> > > >
> > > > The GRE item was here even before the MPLSoGRE support
> > >
> > > Yes, this bug has existed before adding MPLSoGRE support.
> > >
> > > > so I think that this is not the correct fix and even that it can
> > > > hurt the support of GRE for the current customers use it.
> > >
> > > How can it hurt? Please clarify.
> >
> > Someone who uses the next flow and have not the new verbs version of MPLS:
> >  	flow create 0 ingress pattern gre / ipv4 src is X / end actions ...
> > 	ipv4 src or any other inner specifications.
> >
> > This flow will probably get any supported tunnel packets with inner ipv4 src =
> X.
> 
> Do you think we should compromise? 

For sure, no,
I'm just want the correct fix, like you :) 

>This is logically wrong for sure. Let me
> give you a specific example.
> 
> If I create the following two flows,
> 
>   flow create 0 ingress pattern vxlan vni is 10 / end actions queue index 3 /
> mark id 10 / end
>   flow create 0 ingress pattern vxlan vni is 20 / end actions queue index 3 /
> mark id 20 / end
> 
> The following two packets will match correctly and have flow ID (10 and 20)
> according to VNI.
> 
>   Ether()/IP()/UDP()/VXLAN(vni=10)/Ether()/IPv6()
>   Ether()/IP()/UDP()/VXLAN(vni=20)/Ether()/IPv6()
> 
> However, if three flows are created as follows,
> 
>   flow create 0 ingress pattern gre / ipv6 / end actions queue index 3 / mark id
> 2 / end
>   flow create 0 ingress pattern vxlan vni is 10 / end actions queue index 3 /
> mark id 10 / end
>   flow create 0 ingress pattern vxlan vni is 20 / end actions queue index 3 /
> mark id 20 / end
> 
> The packets will hit the first flow regardless of VNI and have wrong flow ID.
> That's why I have to drop this specific GRE case. Whoever is using this kind of
> GRE flow, that's buggy anyway. They have to know it and change it.

I have just checked, the next flow under MPLS support doesn't get neither GRE packet nor non GRE packet:
flow create 0 ingress pattern gre / end actions queue index 3 / mark id
 2 / end

The issue is that in the first implementation of GRE tunnel we didn't forced L3 next protocol to be GRE in this case because L3 is not in the pattern.

> > It may be enough for the current user (which probably use only 1 tunnel type
> at a certain time).
> 
> Router/switch-like applications can have multiple tunnels for sure. I'm not sure
> who 'the current user' is but I don't think we can make such an assumption.
> I don't want to allow users create faulty flows.

I never take such like assumption, just saying...(you can forget it)

The point is that we may have a customer which the current behavior of 
flow create 0 ingress pattern gre / ipv6 .../ end actions queue index 3 / mark id
2 / end
is good for him and now after this fix it will fail in port creation(low probability).

> 
> > > > Looks like you must specify at least 1 spec in the GRE to apply it
> > > > correctly as you did for VXLAN, Can you try empty vxlan and fully
> > > > gre (with protocol field)?
> > >
> > > That's exactly the reason why I'm taking this out. If you look at
> > > the code, it doesn't even set any field for GRE if
> > > HAVE_IBV_DEVICE_MPLS_SUPPORT isn't supported. Thus, it is considered
> > > as a wildcard (all-matching) rule. But if it has
> HAVE_IBV_DEVICE_MPLS_SUPPORT, such pattern can be allowed.
> >
> > Yes, so your GRE flow will not work even if you have MPLS support.
> 
> I'm not sure what you meant but with IBV MPLS support, I think
> IBV_FLOW_SPEC_GRE will make things right. Without the support,
> IBV_FLOW_SPEC_VXLAN_TUNNEL is even set for GRE flows.
> 
> > I think the issue is generally in all the items:
> > You should not configure them if they miss both at least one
> > self-specification or item which points to them by "next protocol" field.
> >
> > In case of VXLAN tunnels we just don't allow them without
> > self-specification, In case of gre we force the next protocol of the previous
> item but only when it exists.
> > In case of eth(inner),vlan,ipv4,ipv6,udp,tcp we don't force anything.
> >
> > I think we need a global fix for all, this is probably the root cause.
> 
> Well, the root-cause is that old device/lib doesn't differentiate GRE from VXLAN
> when creating flows.

OK, let's continue with GRE only and will discuss about other protocol later in the next release.

I think that the root cause is at least lack of GRE detection by next protocol field in the outer L3 in case of first GRE item(verbs limitation).
We can remove this option at all or to fix it by forcing L3 next protocol = GRE in this case as done when we have L3 item before the GRE.