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

[Matan Azrad <matan@mellanox.com>]

Hi Yongseok
 + Steven 

 From: Yongseok Koh
> On Tue, May 22, 2018 at 10:36:43PM -0700, Matan Azrad wrote:
> > Hi Yongseok
> >
> > From:  Yongseok Koh
> > > Creating a flow having pattern from the middle of a packet is
> > > allowed. For example,
> > >
> > >   testpmd> flow create 0 ingress pattern vxlan vni is 20 / end actions ...
> > >
> > > Device can parse GRE header but without proper support from library
> > > and firmware (HAVE_IBV_DEVICE_MPLS_SUPPORT), a field in GRE header
> > > can't be specified when creating a rule. As a result, the following
> > > rule will be interpreted as a wildcard rule, which always matches any
> packet.
> > >
> > >   testpmd> flow create 0 ingress pattern gre / end actions ...
> > > Fixes: 96c6c65a10d2 ("net/mlx5: support GRE tunnel flow")
> > > Fixes: 1f106da2bf7b ("net/mlx5: support MPLS-in-GRE and
> > > MPLS-in-UDP")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_flow.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index 994be05be..526fe6b0e 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -330,9 +330,11 @@ static const enum rte_flow_action_type
> > > valid_actions[] = {  static const struct mlx5_flow_items mlx5_flow_items[] =
> {
> > >  	[RTE_FLOW_ITEM_TYPE_END] = {
> > >  		.items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > +#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.
It may be enough for the current user (which probably use only 1 tunnel type at a certain time).

> > 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 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.

> 
> Having pattern 'vxlan' without vni isn't allowed by mlx5 PMD because zero VNI
> is never accepted.
> 
> Thanks,
> Yongseok
> 
> > > +			       RTE_FLOW_ITEM_TYPE_GRE,
> > > +#endif
> > >  			       RTE_FLOW_ITEM_TYPE_VXLAN,
> > > -			       RTE_FLOW_ITEM_TYPE_VXLAN_GPE,
> > > -			       RTE_FLOW_ITEM_TYPE_GRE),
> > > +			       RTE_FLOW_ITEM_TYPE_VXLAN_GPE),
> > >  	},
> > >  	[RTE_FLOW_ITEM_TYPE_ETH] = {
> > >  		.items = ITEMS(RTE_FLOW_ITEM_TYPE_VLAN,
> >
> >
> >