From: Yongseok Koh <yskoh@mellanox.com>
To: Shahaf Shuler <shahafs@mellanox.com>
Cc: Matan Azrad <matan@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix sanity check for MPLS-in-GRE
Date: Mon, 6 Aug 2018 11:36:34 -0700 [thread overview]
Message-ID: <20180806183632.GA7392@yongseok-MBP.local> (raw)
In-Reply-To: <DB7PR05MB44269E1D373A6D1200434AA1C3210@DB7PR05MB4426.eurprd05.prod.outlook.com>
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
prev parent reply other threads:[~2018-08-06 18:36 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-03 22:00 Yongseok Koh
2018-08-05 6:41 ` Matan Azrad
2018-08-05 11:17 ` Shahaf Shuler
2018-08-06 18:36 ` Yongseok Koh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180806183632.GA7392@yongseok-MBP.local \
--to=yskoh@mellanox.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=shahafs@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).