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: sta...@dpdk.org
> > >
> > > Signed-off-by: Yongseok Koh <ys...@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,
> >
> >
> >
