diff mbox series

net/mlx5: fix sanity check for MPLS-in-GRE

Message ID 20180803220056.22840-1-yskoh@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Shahaf Shuler
Headers
Series net/mlx5: fix sanity check for MPLS-in-GRE |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Yongseok Koh Aug. 3, 2018, 10 p.m. UTC 
  Multiple tunnel isn't allowed but MPLS over GRE should be accepted.

Fixes: a4a5cd21d20a ("net/mlx5: add flow MPLS item")

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Matan Azrad Aug. 5, 2018, 6:41 a.m. UTC | #1 
Hi Koh

From: Yongseok Koh
> Multiple tunnel isn't allowed but MPLS over GRE should be accepted.
> 
> Fixes: a4a5cd21d20a ("net/mlx5: add flow MPLS item")
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c index
> b7500ec9d6..ca4625b699 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1778,7 +1778,9 @@ mlx5_flow_item_mpls(const struct rte_flow_item
> *item __rte_unused,
>  					  item,
>  					  "protocol filtering not compatible"
>  					  " with MPLS layer");
> -	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL)
> +	/* Multi-tunnel isn't allowed but MPLS over GRE is an exception. */
> +	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL &&
> +	    (flow->layers & MLX5_FLOW_LAYER_GRE) !=

This check is not fully correct because the GRE item must be the last valid item before the mpls, so the next flow

eth / ipv4 / gre / ipv4 / mpls

is not valid.

But the next flows are valid:
eth / ipv4 / gre / mpls
eth / ipv4 / gre / void / mpls
eth / ipv4 / gre / void / void / void / mpls



> MLX5_FLOW_LAYER_GRE)
>  		return rte_flow_error_set(error, ENOTSUP,
>  					  RTE_FLOW_ERROR_TYPE_ITEM,
>  					  item,
> --
> 2.11.0
Shahaf Shuler Aug. 5, 2018, 11:17 a.m. UTC | #2 
Sunday, August 5, 2018 9:41 AM, Matan Azrad:
> Subject: RE: [dpdk-dev] [PATCH] net/mlx5: fix sanity check for MPLS-in-GRE
> 
> Hi Koh
> 
> From: Yongseok Koh
> > Multiple tunnel isn't allowed but MPLS over GRE should be accepted.
> >
> > Fixes: a4a5cd21d20a ("net/mlx5: add flow MPLS item")
> >
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index
> > b7500ec9d6..ca4625b699 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -1778,7 +1778,9 @@ mlx5_flow_item_mpls(const struct
> rte_flow_item
> > *item __rte_unused,
> >  					  item,
> >  					  "protocol filtering not compatible"
> >  					  " with MPLS layer");
> > -	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL)
> > +	/* Multi-tunnel isn't allowed but MPLS over GRE is an exception. */
> > +	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL &&
> > +	    (flow->layers & MLX5_FLOW_LAYER_GRE) !=
> 
> This check is not fully correct because the GRE item must be the last valid
> item before the mpls, so the next flow
> 
> eth / ipv4 / gre / ipv4 / mpls
> 
> is not valid.
> 
> But the next flows are valid:
> eth / ipv4 / gre / mpls
> eth / ipv4 / gre / void / mpls
> eth / ipv4 / gre / void / void / void / mpls

Spoke w/ Matan on it. 
It is correct this patch is not complete, however considering the release schedule it is better to have it in than giveup the MPLS-in-GRE support for 18.08.

Hence applying this patch and keep track on subsequent one to fully detect all cases.

Applied to next-net-mlx, thanks. 

> 
> 
> 
> > MLX5_FLOW_LAYER_GRE)
> >  		return rte_flow_error_set(error, ENOTSUP,
> >  					  RTE_FLOW_ERROR_TYPE_ITEM,
> >  					  item,
> > --
> > 2.11.0
Yongseok Koh Aug. 6, 2018, 6:36 p.m. UTC | #3 
On Sun, Aug 05, 2018 at 04:17:21AM -0700, Shahaf Shuler wrote:
> Sunday, August 5, 2018 9:41 AM, Matan Azrad:
> > Subject: RE: [dpdk-dev] [PATCH] net/mlx5: fix sanity check for MPLS-in-GRE
> > 
> > Hi Koh
> > 
> > From: Yongseok Koh
> > > Multiple tunnel isn't allowed but MPLS over GRE should be accepted.
> > >
> > > Fixes: a4a5cd21d20a ("net/mlx5: add flow MPLS item")
> > >
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index
> > > b7500ec9d6..ca4625b699 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -1778,7 +1778,9 @@ mlx5_flow_item_mpls(const struct
> > rte_flow_item
> > > *item __rte_unused,
> > >  					  item,
> > >  					  "protocol filtering not compatible"
> > >  					  " with MPLS layer");
> > > -	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL)
> > > +	/* Multi-tunnel isn't allowed but MPLS over GRE is an exception. */
> > > +	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL &&
> > > +	    (flow->layers & MLX5_FLOW_LAYER_GRE) !=
> > 
> > This check is not fully correct because the GRE item must be the last valid
> > item before the mpls, so the next flow
> > 
> > eth / ipv4 / gre / ipv4 / mpls
> > 
> > is not valid.
> > 
> > But the next flows are valid:
> > eth / ipv4 / gre / mpls
> > eth / ipv4 / gre / void / mpls
> > eth / ipv4 / gre / void / void / void / mpls
> 
> Spoke w/ Matan on it. 
> It is correct this patch is not complete, however considering the release schedule it is better to have it in than giveup the MPLS-in-GRE support for 18.08.
> 
> Hence applying this patch and keep track on subsequent one to fully detect all cases.
> 
> Applied to next-net-mlx, thanks. 

Good to see it is merged, but small comment.

This patch is correct and complete by itself. This patch wasn't intended to add
all the lacking sanity checks complementing the current flow engine but only to
fix a false negative in order to allow MPLSoGRE creation. There're a few more
other cases which are not filtered by validation code. For example, the
following is also accepted by PMD but HW refuses.

	pattern eth / ipv4 / udp / gre / end

We don't want to push many lines of code in RC stage but bug fixes. As the flow
engine doesn't store the previous item, it would've needed quite a few lines of
code to address such lacking sanity checks. Those should be done by additional
patch for the next (stable) release if needed.

Thanks,
Yongseok
diff mbox series

Patch

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index b7500ec9d6..ca4625b699 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -1778,7 +1778,9 @@  mlx5_flow_item_mpls(const struct rte_flow_item *item __rte_unused,
 					  item,
 					  "protocol filtering not compatible"
 					  " with MPLS layer");
-	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL)
+	/* Multi-tunnel isn't allowed but MPLS over GRE is an exception. */
+	if (flow->layers & MLX5_FLOW_LAYER_TUNNEL &&
+	    (flow->layers & MLX5_FLOW_LAYER_GRE) != MLX5_FLOW_LAYER_GRE)
 		return rte_flow_error_set(error, ENOTSUP,
 					  RTE_FLOW_ERROR_TYPE_ITEM,
 					  item,