* [dpdk-stable] [PATCH] net/sfc: cut non VLAN ID bits from TCI @ 2018-06-29 15:23 Andrew Rybchenko 2018-07-04 17:43 ` Ferruh Yigit 2018-07-05 11:14 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil 0 siblings, 2 replies; 5+ messages in thread From: Andrew Rybchenko @ 2018-06-29 15:23 UTC (permalink / raw) To: dev; +Cc: stable TCI may contain PCP or DEI bits. Matching of these bits is not supported, but the bits still may be set in specification value and not covered by mask. So, these bits should be ignored. Fixes: 894080975e1e ("net/sfc: support VLAN in flow API filters") Cc: stable@dpdk.org Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> Reviewed-by: Roman Zhukov <roman.zhukov@oktetlabs.ru> --- drivers/net/sfc/sfc_flow.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c index 5613d59a9..18387415e 100644 --- a/drivers/net/sfc/sfc_flow.c +++ b/drivers/net/sfc/sfc_flow.c @@ -371,7 +371,8 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item, * the outer tag and the next matches the inner tag. */ if (mask->tci == supp_mask.tci) { - vid = rte_bswap16(spec->tci); + /* Apply mask to keep VID only */ + vid = rte_bswap16(spec->tci & mask->tci); if (!(efx_spec->efs_match_flags & EFX_FILTER_MATCH_OUTER_VID)) { -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [PATCH] net/sfc: cut non VLAN ID bits from TCI 2018-06-29 15:23 [dpdk-stable] [PATCH] net/sfc: cut non VLAN ID bits from TCI Andrew Rybchenko @ 2018-07-04 17:43 ` Ferruh Yigit 2018-07-05 11:14 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil 1 sibling, 0 replies; 5+ messages in thread From: Ferruh Yigit @ 2018-07-04 17:43 UTC (permalink / raw) To: Andrew Rybchenko, dev; +Cc: stable On 6/29/2018 4:23 PM, Andrew Rybchenko wrote: > TCI may contain PCP or DEI bits. Matching of these bits is not > supported, but the bits still may be set in specification value and > not covered by mask. So, these bits should be ignored. > > Fixes: 894080975e1e ("net/sfc: support VLAN in flow API filters") > Cc: stable@dpdk.org > > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> > Reviewed-by: Roman Zhukov <roman.zhukov@oktetlabs.ru> Applied to dpdk-next-net/master, thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/sfc: cut non VLAN ID bits from TCI 2018-06-29 15:23 [dpdk-stable] [PATCH] net/sfc: cut non VLAN ID bits from TCI Andrew Rybchenko 2018-07-04 17:43 ` Ferruh Yigit @ 2018-07-05 11:14 ` Adrien Mazarguil 2018-07-05 19:23 ` Adrien Mazarguil 1 sibling, 1 reply; 5+ messages in thread From: Adrien Mazarguil @ 2018-07-05 11:14 UTC (permalink / raw) To: Andrew Rybchenko; +Cc: dev, stable On Fri, Jun 29, 2018 at 04:23:31PM +0100, Andrew Rybchenko wrote: > TCI may contain PCP or DEI bits. Matching of these bits is not > supported, but the bits still may be set in specification value and > not covered by mask. So, these bits should be ignored. > > Fixes: 894080975e1e ("net/sfc: support VLAN in flow API filters") > Cc: stable@dpdk.org > > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> > Reviewed-by: Roman Zhukov <roman.zhukov@oktetlabs.ru> > > --- > drivers/net/sfc/sfc_flow.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c > index 5613d59a9..18387415e 100644 > --- a/drivers/net/sfc/sfc_flow.c > +++ b/drivers/net/sfc/sfc_flow.c > @@ -371,7 +371,8 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item, > * the outer tag and the next matches the inner tag. > */ > if (mask->tci == supp_mask.tci) { > - vid = rte_bswap16(spec->tci); > + /* Apply mask to keep VID only */ > + vid = rte_bswap16(spec->tci & mask->tci); > > if (!(efx_spec->efs_match_flags & > EFX_FILTER_MATCH_OUTER_VID)) { I think there is an issue with this patch when spec->mask is user-provided, PMDs have to trust it. They must not simply ignore bits they cannot handle but explicitly reject the flow rule for correctness. Most devices cannot match PCP and DEI, this is why the default TCI mask was modified in v18.05 by commit 0730ab674cb1 ("ethdev: fix default VLAN TCI mask in flow API") to cover the VID part only. I wrote a helper for mlx5 to help with such basic sanity checks on pattern items in a generic(ish) fashion, maybe you could reuse something. Have a look at mlx5_nl_flow_item_mask() [1]. [1] "net/mlx5: add L2-L4 pattern items to switch flow rules" https://mails.dpdk.org/archives/dev/2018-June/105579.html -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/sfc: cut non VLAN ID bits from TCI 2018-07-05 11:14 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil @ 2018-07-05 19:23 ` Adrien Mazarguil 2018-07-07 16:12 ` Andrew Rybchenko 0 siblings, 1 reply; 5+ messages in thread From: Adrien Mazarguil @ 2018-07-05 19:23 UTC (permalink / raw) To: Andrew Rybchenko; +Cc: dev, stable On Thu, Jul 05, 2018 at 01:14:49PM +0200, Adrien Mazarguil wrote: > On Fri, Jun 29, 2018 at 04:23:31PM +0100, Andrew Rybchenko wrote: > > TCI may contain PCP or DEI bits. Matching of these bits is not > > supported, but the bits still may be set in specification value and > > not covered by mask. So, these bits should be ignored. > > > > Fixes: 894080975e1e ("net/sfc: support VLAN in flow API filters") > > Cc: stable@dpdk.org > > > > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> > > Reviewed-by: Roman Zhukov <roman.zhukov@oktetlabs.ru> > > > > --- > > drivers/net/sfc/sfc_flow.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c > > index 5613d59a9..18387415e 100644 > > --- a/drivers/net/sfc/sfc_flow.c > > +++ b/drivers/net/sfc/sfc_flow.c > > @@ -371,7 +371,8 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item, > > * the outer tag and the next matches the inner tag. > > */ > > if (mask->tci == supp_mask.tci) { > > - vid = rte_bswap16(spec->tci); > > + /* Apply mask to keep VID only */ > > + vid = rte_bswap16(spec->tci & mask->tci); > > > > if (!(efx_spec->efs_match_flags & > > EFX_FILTER_MATCH_OUTER_VID)) { > > I think there is an issue with this patch when spec->mask is user-provided, > PMDs have to trust it. They must not simply ignore bits they cannot handle > but explicitly reject the flow rule for correctness. > > Most devices cannot match PCP and DEI, this is why the default TCI mask was > modified in v18.05 by commit 0730ab674cb1 ("ethdev: fix default VLAN TCI > mask in flow API") to cover the VID part only. > > I wrote a helper for mlx5 to help with such basic sanity checks on pattern > items in a generic(ish) fashion, maybe you could reuse something. Have a > look at mlx5_nl_flow_item_mask() [1]. > > [1] "net/mlx5: add L2-L4 pattern items to switch flow rules" > https://mails.dpdk.org/archives/dev/2018-June/105579.html Seems like I misread the original patch and PMD code, both are in fact correct. Please ignore my previous comment, sorry for the noise! -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/sfc: cut non VLAN ID bits from TCI 2018-07-05 19:23 ` Adrien Mazarguil @ 2018-07-07 16:12 ` Andrew Rybchenko 0 siblings, 0 replies; 5+ messages in thread From: Andrew Rybchenko @ 2018-07-07 16:12 UTC (permalink / raw) To: Adrien Mazarguil, Andrew Rybchenko; +Cc: dev, stable On 05.07.2018 22:23, Adrien Mazarguil wrote: > On Thu, Jul 05, 2018 at 01:14:49PM +0200, Adrien Mazarguil wrote: >> On Fri, Jun 29, 2018 at 04:23:31PM +0100, Andrew Rybchenko wrote: >>> TCI may contain PCP or DEI bits. Matching of these bits is not >>> supported, but the bits still may be set in specification value and >>> not covered by mask. So, these bits should be ignored. >>> >>> Fixes: 894080975e1e ("net/sfc: support VLAN in flow API filters") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> >>> Reviewed-by: Roman Zhukov <roman.zhukov@oktetlabs.ru> >>> >>> --- >>> drivers/net/sfc/sfc_flow.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/sfc/sfc_flow.c b/drivers/net/sfc/sfc_flow.c >>> index 5613d59a9..18387415e 100644 >>> --- a/drivers/net/sfc/sfc_flow.c >>> +++ b/drivers/net/sfc/sfc_flow.c >>> @@ -371,7 +371,8 @@ sfc_flow_parse_vlan(const struct rte_flow_item *item, >>> * the outer tag and the next matches the inner tag. >>> */ >>> if (mask->tci == supp_mask.tci) { >>> - vid = rte_bswap16(spec->tci); >>> + /* Apply mask to keep VID only */ >>> + vid = rte_bswap16(spec->tci & mask->tci); >>> >>> if (!(efx_spec->efs_match_flags & >>> EFX_FILTER_MATCH_OUTER_VID)) { >> I think there is an issue with this patch when spec->mask is user-provided, >> PMDs have to trust it. They must not simply ignore bits they cannot handle >> but explicitly reject the flow rule for correctness. >> >> Most devices cannot match PCP and DEI, this is why the default TCI mask was >> modified in v18.05 by commit 0730ab674cb1 ("ethdev: fix default VLAN TCI >> mask in flow API") to cover the VID part only. >> >> I wrote a helper for mlx5 to help with such basic sanity checks on pattern >> items in a generic(ish) fashion, maybe you could reuse something. Have a >> look at mlx5_nl_flow_item_mask() [1]. >> >> [1] "net/mlx5: add L2-L4 pattern items to switch flow rules" >> https://mails.dpdk.org/archives/dev/2018-June/105579.html > Seems like I misread the original patch and PMD code, both are in fact > correct. > > Please ignore my previous comment, sorry for the noise! No problem. Many thanks for reviewing it. Andrew. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-07 16:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-29 15:23 [dpdk-stable] [PATCH] net/sfc: cut non VLAN ID bits from TCI Andrew Rybchenko 2018-07-04 17:43 ` Ferruh Yigit 2018-07-05 11:14 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil 2018-07-05 19:23 ` Adrien Mazarguil 2018-07-07 16:12 ` Andrew Rybchenko
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).