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

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

[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
+			       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,
-- 
2.11.0

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

[Matan Azrad]

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

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)?

> +			       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,

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

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

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

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

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

[Matan Azrad]

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

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

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

Do you think we should compromise? 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.

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

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


Thanks,
Yongseok

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

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

[Xueming(Steven) Li]

Hi Koh,

> -----Original Message-----
> From: Yongseok Koh
> Sent: Thursday, May 24, 2018 2:34 AM
> To: Matan Azrad <matan@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>; Nélio
> Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; stable@dpdk.org; Xueming(Steven) Li
> <xuemingl@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix GRE flow rule
> 
> 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
> > > > > 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.
> 
> Do you think we should compromise? 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.

Creating rules start with Vxlan or GRE is dangers, outer IP protocol or UDP dst port is not tested.
Such rule mainly exist in lab.

> 
> > 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 know that GRE has been deployed.

> 
> > > > 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.
> 
> 
> Thanks,
> Yongseok
> 
> > > 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,
> > > >
> > > >
> > > >

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

[Matan Azrad]

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.
 

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

[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 protocol number in outer IP header but specifying from
GRE header can't differentiate it from VxLAN tunnel. 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>
---

v2:
* amend commit message.
* remove GRE entry from the head item regardless of HW support.

 drivers/net/mlx5/mlx5_flow.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 994be05be..adb995f0d 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -331,8 +331,7 @@ static const struct mlx5_flow_items mlx5_flow_items[] = {
 	[RTE_FLOW_ITEM_TYPE_END] = {
 		.items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
 			       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,
-- 
2.11.0

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

[Yongseok Koh]

> On May 24, 2018, at 10:56 AM, Yongseok Koh <yskoh@mellanox.com> wrote:
> 
> 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 protocol number in outer IP header but specifying from
> GRE header can't differentiate it from VxLAN tunnel. 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>
> ---
> 
> v2:
> * amend commit message.
> * remove GRE entry from the head item regardless of HW support.
> 
> drivers/net/mlx5/mlx5_flow.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 994be05be..adb995f0d 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -331,8 +331,7 @@ static const struct mlx5_flow_items mlx5_flow_items[] = {
> 	[RTE_FLOW_ITEM_TYPE_END] = {
> 		.items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> 			       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,
> -- 

Taking back this patch as the entire flow engine will be remodeled.
But this will be sent again for stable branches.

Thanks,
Yongseok