Re: [dpdk-dev] [PATCH] net/mlx5: prohibit wildcard match for GRE protocol

[Yongseok Koh <yskoh@mellanox.com>]

On Sun, Aug 05, 2018 at 12:01:01AM -0700, Matan Azrad wrote:
> Hi
> More info below...
> 
> From: Matan Azrad
> > Hi Koh
> > 
> > From: Yongseok Koh
> > > Currently, driver sets full mask (0xffff) for GRE protocol to device.
> > > Until it is changed, PMD can't support wildcard match.
> > >
> > > Fixes: f4b901a46aec ("net/mlx5: add flow GRE item")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_flow.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index
> > > b7500ec9d6..83ac6cbb85 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -1690,8 +1690,12 @@ mlx5_flow_item_gre(const struct rte_flow_item
> > > *item,
> > >  					  RTE_FLOW_ERROR_TYPE_ITEM,
> > >  					  item,
> > >  					  "L3 Layer is missing");
> > > -	if (!mask)
> > > -		mask = &rte_flow_item_gre_mask;
> > > +	if (!mask || mask->protocol != 0xffff)
> > 
> > Why do you check !mask again, I think it's redundant.. no?
> > 
> > I think we can support also mask->protocol = 0:
> > In this case we just need to not specify a spec.

Right. My original intention is to force users to use 'is' or 'spec' with full
mask being specified not depending on the default mask. But error message isn't
enough. And users know what the default mask is.

> Just to be more accurate:
> 
> The case of !mask led to mask->protocol = 0xffff before this patch, so it's ok and should be supported.
> In addition, we can support also mask->protocol = 0 (means match all the gre protocol values) by remaining empty spec\mask to the verbs.
> It can be something like this;
> 	if (!mask)
> 		mask = &rte_flow_item_gre_mask;
> 	if (mask->protocol == 0)
> 		spec = NULL;

This shouldn't work. If spec is null, ibv_flow_spec_gre will have zeroed val and
mask. And this will lead to protocol being zero with full mask in the driver.
Also, we have one lacking validation check here - driver doesn't support
c_ks_res0_ver yet.

> 	else if (mask->protocol != 0xffff)
> 		ERROR!
> 
> I checked the next flow:
> eth / ipv4 / gre
> 
> It matched all the gre traffic(MLNX_OFED_LINUX-4.4-2.0.4.0).

I figured out the reason for your false positive. This issue has already been
fixed in the latest MOFED release and the patch is pending for upstream [1].

We should void this temp patch but document it appropriately instead as it can
work well with the MOFED release but not with upstream kernel. Let's take it
offline.


[1] https://patchwork.kernel.org/patch/10498671/


Thanks,
Yongseok