patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations
@ 2018-08-02 10:33 Matan Azrad
  2018-08-02 12:03 ` Wiles, Keith
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Matan Azrad @ 2018-08-02 10:33 UTC (permalink / raw)
  To: Keith Wiles; +Cc: Ophir Munk, dev, stable

The rte_flow meaning of zero flow mask configuration is to match all
the range of the item value.
For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
should much all the ipv4 traffic from the rte_flow API perspective.

>From some kernel perspectives the above rule means to ignore all the
ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).

Due to the fact that the tap PMD should provide the rte_flow meaning,
it is necessary to ignore the spec in case the mask is zero when it
forwards such like flows to the kernel.
So, the above rule should be translated to eth / ipv4 to get the
correct meaning.

Ignore spec configurations when the mask is zero.

Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 6b60e6d..993e6f6 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -537,7 +537,8 @@ tap_flow_create_eth(const struct rte_flow_item *item, void *data)
 	if (!flow)
 		return 0;
 	msg = &flow->msg;
-	if (!is_zero_ether_addr(&spec->dst)) {
+	if (!is_zero_ether_addr(&spec->dst) &&
+	    !is_zero_ether_addr(&mask->dst)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST, ETHER_ADDR_LEN,
 			   &spec->dst.addr_bytes);
 		tap_nlattr_add(&msg->nh,
@@ -651,13 +652,13 @@ tap_flow_create_ipv4(const struct rte_flow_item *item, void *data)
 		info->eth_type = htons(ETH_P_IP);
 	if (!spec)
 		return 0;
-	if (spec->hdr.dst_addr) {
+	if (spec->hdr.dst_addr && mask->hdr.dst_addr) {
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
 			     spec->hdr.dst_addr);
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST_MASK,
 			     mask->hdr.dst_addr);
 	}
-	if (spec->hdr.src_addr) {
+	if (spec->hdr.src_addr && mask->hdr.src_addr) {
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
 			     spec->hdr.src_addr);
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC_MASK,
@@ -707,13 +708,15 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void *data)
 		info->eth_type = htons(ETH_P_IPV6);
 	if (!spec)
 		return 0;
-	if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
+	if (memcmp(spec->hdr.dst_addr, empty_addr, 16) &&
+	    memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
 			   sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST_MASK,
 			   sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
 	}
-	if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
+	if (memcmp(spec->hdr.src_addr, empty_addr, 16) &&
+	    memcmp(mask->hdr.src_addr, empty_addr, 16)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
 			   sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC_MASK,
-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-02 10:33 [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations Matan Azrad
@ 2018-08-02 12:03 ` Wiles, Keith
  2018-08-02 14:27 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil
  2018-08-06 10:58 ` [dpdk-stable] [PATCH v2] " Matan Azrad
  2 siblings, 0 replies; 11+ messages in thread
From: Wiles, Keith @ 2018-08-02 12:03 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Ophir Munk, dev, stable



> On Aug 2, 2018, at 5:33 AM, Matan Azrad <matan@mellanox.com> wrote:
> 
> The rte_flow meaning of zero flow mask configuration is to match all
> the range of the item value.
> For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> should much all the ipv4 traffic from the rte_flow API perspective.
> 
> From some kernel perspectives the above rule means to ignore all the
> ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> 
> Due to the fact that the tap PMD should provide the rte_flow meaning,
> it is necessary to ignore the spec in case the mask is zero when it
> forwards such like flows to the kernel.
> So, the above rule should be translated to eth / ipv4 to get the
> correct meaning.
> 
> Ignore spec configurations when the mask is zero.
> 
> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Acked-by: Keith Wiles<keith.wiles@intel.com>

Regards,
Keith

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-02 10:33 [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations Matan Azrad
  2018-08-02 12:03 ` Wiles, Keith
@ 2018-08-02 14:27 ` Adrien Mazarguil
  2018-08-02 17:52   ` Matan Azrad
  2018-08-06 10:58 ` [dpdk-stable] [PATCH v2] " Matan Azrad
  2 siblings, 1 reply; 11+ messages in thread
From: Adrien Mazarguil @ 2018-08-02 14:27 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Keith Wiles, Ophir Munk, dev, stable

On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> The rte_flow meaning of zero flow mask configuration is to match all
> the range of the item value.
> For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> should much all the ipv4 traffic from the rte_flow API perspective.
> 
> From some kernel perspectives the above rule means to ignore all the
> ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> 
> Due to the fact that the tap PMD should provide the rte_flow meaning,
> it is necessary to ignore the spec in case the mask is zero when it
> forwards such like flows to the kernel.
> So, the above rule should be translated to eth / ipv4 to get the
> correct meaning.
> 
> Ignore spec configurations when the mask is zero.

I would go further, one should be able to match IP address 0.0.0.0 for
instance. The PMD should only trust the mask on all fields without looking
at spec. See below for suggestions.

> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/tap/tap_flow.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index 6b60e6d..993e6f6 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -537,7 +537,8 @@ tap_flow_create_eth(const struct rte_flow_item *item, void *data)
>  	if (!flow)
>  		return 0;
>  	msg = &flow->msg;
> -	if (!is_zero_ether_addr(&spec->dst)) {
> +	if (!is_zero_ether_addr(&spec->dst) &&

This check should be removed.

> +	    !is_zero_ether_addr(&mask->dst)) {
>  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST, ETHER_ADDR_LEN,
>  			   &spec->dst.addr_bytes);
>  		tap_nlattr_add(&msg->nh,
> @@ -651,13 +652,13 @@ tap_flow_create_ipv4(const struct rte_flow_item *item, void *data)
>  		info->eth_type = htons(ETH_P_IP);
>  	if (!spec)
>  		return 0;
> -	if (spec->hdr.dst_addr) {
> +	if (spec->hdr.dst_addr && mask->hdr.dst_addr) {

Ditto (before &&).

>  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
>  			     spec->hdr.dst_addr);
>  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST_MASK,
>  			     mask->hdr.dst_addr);
>  	}
> -	if (spec->hdr.src_addr) {
> +	if (spec->hdr.src_addr && mask->hdr.src_addr) {

Ditto.

>  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
>  			     spec->hdr.src_addr);
>  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC_MASK,
> @@ -707,13 +708,15 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void *data)
>  		info->eth_type = htons(ETH_P_IPV6);
>  	if (!spec)
>  		return 0;
> -	if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
> +	if (memcmp(spec->hdr.dst_addr, empty_addr, 16) &&

Ditto.

> +	    memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
>  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
>  			   sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
>  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST_MASK,
>  			   sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
>  	}
> -	if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
> +	if (memcmp(spec->hdr.src_addr, empty_addr, 16) &&

Ditto.

> +	    memcmp(mask->hdr.src_addr, empty_addr, 16)) {
>  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
>  			   sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
>  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC_MASK,
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-02 14:27 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil
@ 2018-08-02 17:52   ` Matan Azrad
  2018-08-03  8:20     ` Adrien Mazarguil
  0 siblings, 1 reply; 11+ messages in thread
From: Matan Azrad @ 2018-08-02 17:52 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: Keith Wiles, Ophir Munk, dev, stable

Hi Adrien

From: Adrien Mazarguil
> On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > The rte_flow meaning of zero flow mask configuration is to match all
> > the range of the item value.
> > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > should much all the ipv4 traffic from the rte_flow API perspective.
> >
> > From some kernel perspectives the above rule means to ignore all the
> > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> >
> > Due to the fact that the tap PMD should provide the rte_flow meaning,
> > it is necessary to ignore the spec in case the mask is zero when it
> > forwards such like flows to the kernel.
> > So, the above rule should be translated to eth / ipv4 to get the
> > correct meaning.
> >
> > Ignore spec configurations when the mask is zero.
> 
> I would go further, one should be able to match IP address 0.0.0.0 for instance.
> The PMD should only trust the mask on all fields without looking at spec.

The PMD should convert the RTE flow API to the device configuration,
So I can think on scenarios that the PMD should look on spec.

 See
> below for suggestions.
> 
> > Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and
> > actions")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  drivers/net/tap/tap_flow.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > index 6b60e6d..993e6f6 100644
> > --- a/drivers/net/tap/tap_flow.c
> > +++ b/drivers/net/tap/tap_flow.c
> > @@ -537,7 +537,8 @@ tap_flow_create_eth(const struct rte_flow_item
> *item, void *data)
> >  	if (!flow)
> >  		return 0;
> >  	msg = &flow->msg;
> > -	if (!is_zero_ether_addr(&spec->dst)) {
> > +	if (!is_zero_ether_addr(&spec->dst) &&
> 
> This check should be removed.

I don't know why we need this check, and the below checks
So it should be tested before the change.
It may be a different issue.

> 
> > +	    !is_zero_ether_addr(&mask->dst)) {
> >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST,
> ETHER_ADDR_LEN,
> >  			   &spec->dst.addr_bytes);
> >  		tap_nlattr_add(&msg->nh,
> > @@ -651,13 +652,13 @@ tap_flow_create_ipv4(const struct rte_flow_item
> *item, void *data)
> >  		info->eth_type = htons(ETH_P_IP);
> >  	if (!spec)
> >  		return 0;
> > -	if (spec->hdr.dst_addr) {
> > +	if (spec->hdr.dst_addr && mask->hdr.dst_addr) {
> 
> Ditto (before &&).
> 
> >  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
> >  			     spec->hdr.dst_addr);
> >  		tap_nlattr_add32(&msg->nh,
> TCA_FLOWER_KEY_IPV4_DST_MASK,
> >  			     mask->hdr.dst_addr);
> >  	}
> > -	if (spec->hdr.src_addr) {
> > +	if (spec->hdr.src_addr && mask->hdr.src_addr) {
> 
> Ditto.
> 
> >  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
> >  			     spec->hdr.src_addr);
> >  		tap_nlattr_add32(&msg->nh,
> TCA_FLOWER_KEY_IPV4_SRC_MASK, @@ -707,13
> > +708,15 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void
> *data)
> >  		info->eth_type = htons(ETH_P_IPV6);
> >  	if (!spec)
> >  		return 0;
> > -	if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
> > +	if (memcmp(spec->hdr.dst_addr, empty_addr, 16) &&
> 
> Ditto.
> 
> > +	    memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
> >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
> >  			   sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
> >  		tap_nlattr_add(&msg->nh,
> TCA_FLOWER_KEY_IPV6_DST_MASK,
> >  			   sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
> >  	}
> > -	if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
> > +	if (memcmp(spec->hdr.src_addr, empty_addr, 16) &&
> 
> Ditto.
> 
> > +	    memcmp(mask->hdr.src_addr, empty_addr, 16)) {
> >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
> >  			   sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
> >  		tap_nlattr_add(&msg->nh,
> TCA_FLOWER_KEY_IPV6_SRC_MASK,
> > --
> > 2.7.4
> >
> 
> --
> Adrien Mazarguil
> 6WIND

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-02 17:52   ` Matan Azrad
@ 2018-08-03  8:20     ` Adrien Mazarguil
  2018-08-05  6:10       ` Matan Azrad
  0 siblings, 1 reply; 11+ messages in thread
From: Adrien Mazarguil @ 2018-08-03  8:20 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Keith Wiles, Ophir Munk, dev, stable

Hi Matan,

On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> Hi Adrien
> 
> From: Adrien Mazarguil
> > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > The rte_flow meaning of zero flow mask configuration is to match all
> > > the range of the item value.
> > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > > should much all the ipv4 traffic from the rte_flow API perspective.
> > >
> > > From some kernel perspectives the above rule means to ignore all the
> > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > >
> > > Due to the fact that the tap PMD should provide the rte_flow meaning,
> > > it is necessary to ignore the spec in case the mask is zero when it
> > > forwards such like flows to the kernel.
> > > So, the above rule should be translated to eth / ipv4 to get the
> > > correct meaning.
> > >
> > > Ignore spec configurations when the mask is zero.
> > 
> > I would go further, one should be able to match IP address 0.0.0.0 for instance.
> > The PMD should only trust the mask on all fields without looking at spec.
> 
> The PMD should convert the RTE flow API to the device configuration,
> So I can think on scenarios that the PMD should look on spec.

Obviously the PMD needs to take spec into account. What I meant is that for
each field, spec must be taken into account according to mask only.

For any given field, when mask is empty, don't look at spec, it's like a
wildcard. When mask is full, take spec as is, even if spec only contains
zeroed bits.

User intent in that case is to match a zero value exactly, so it must not
result in a wildcard match. If supported, when mask is partial, masked bits
are also matched exactly, even if these turn out to be a zero
value. Unmasked bits are considered wildcards.

In short, to address both the issue mentioned in the commit log and the one
I'm talking about, you only need to replace "spec" with "mask" in the
original code. More below.

>  See
> > below for suggestions.
> > 
> > > Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and
> > > actions")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  drivers/net/tap/tap_flow.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > > index 6b60e6d..993e6f6 100644
> > > --- a/drivers/net/tap/tap_flow.c
> > > +++ b/drivers/net/tap/tap_flow.c
> > > @@ -537,7 +537,8 @@ tap_flow_create_eth(const struct rte_flow_item
> > *item, void *data)
> > >  	if (!flow)
> > >  		return 0;
> > >  	msg = &flow->msg;
> > > -	if (!is_zero_ether_addr(&spec->dst)) {
> > > +	if (!is_zero_ether_addr(&spec->dst) &&
> > 
> > This check should be removed.
> 
> I don't know why we need this check, and the below checks
> So it should be tested before the change.
> It may be a different issue.
> 
> > 
> > > +	    !is_zero_ether_addr(&mask->dst)) {

Should read:

 if (!is_zero_ether_addr(&mask->dst)) {

> > >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST,
> > ETHER_ADDR_LEN,
> > >  			   &spec->dst.addr_bytes);
> > >  		tap_nlattr_add(&msg->nh,
> > > @@ -651,13 +652,13 @@ tap_flow_create_ipv4(const struct rte_flow_item
> > *item, void *data)
> > >  		info->eth_type = htons(ETH_P_IP);
> > >  	if (!spec)
> > >  		return 0;
> > > -	if (spec->hdr.dst_addr) {
> > > +	if (spec->hdr.dst_addr && mask->hdr.dst_addr) {
> > 
> > Ditto (before &&).

Should read:

 if (mask->hdr.dst_addr) {

> > 
> > >  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
> > >  			     spec->hdr.dst_addr);
> > >  		tap_nlattr_add32(&msg->nh,
> > TCA_FLOWER_KEY_IPV4_DST_MASK,
> > >  			     mask->hdr.dst_addr);
> > >  	}
> > > -	if (spec->hdr.src_addr) {
> > > +	if (spec->hdr.src_addr && mask->hdr.src_addr) {
> > 
> > Ditto.

Should read:

 if (mask->hdr.dst_addr) {

> > >  		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
> > >  			     spec->hdr.src_addr);
> > >  		tap_nlattr_add32(&msg->nh,
> > TCA_FLOWER_KEY_IPV4_SRC_MASK, @@ -707,13
> > > +708,15 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void
> > *data)
> > >  		info->eth_type = htons(ETH_P_IPV6);
> > >  	if (!spec)
> > >  		return 0;
> > > -	if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
> > > +	if (memcmp(spec->hdr.dst_addr, empty_addr, 16) &&
> > 
> > Ditto.

Should read:

 if (memcmp(mask->hdr.dst_addr, empty_addr, 16)) {

> > 
> > > +	    memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
> > >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
> > >  			   sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
> > >  		tap_nlattr_add(&msg->nh,
> > TCA_FLOWER_KEY_IPV6_DST_MASK,
> > >  			   sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
> > >  	}
> > > -	if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
> > > +	if (memcmp(spec->hdr.src_addr, empty_addr, 16) &&
> > 
> > Ditto.

Should read:

 if (memcmp(mask->hdr.src_addr, empty_addr, 16)) {

> > 
> > > +	    memcmp(mask->hdr.src_addr, empty_addr, 16)) {
> > >  		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
> > >  			   sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
> > >  		tap_nlattr_add(&msg->nh,
> > TCA_FLOWER_KEY_IPV6_SRC_MASK,
> > > --
> > > 2.7.4
> > >

The same issue exists with UDP and TCP ports by the way:

 -if (spec->hdr.dst_port & mask->hdr.dst_port)
 +if (mask->hdr.dst_port)

 -if (spec->hdr.src_port & mask->hdr.src_port)
 +if (mask->hdr.src_port)


 -if (spec->hdr.dst_port & mask->hdr.dst_port)
 +if (mask->hdr.dst_port)

 -if (spec->hdr.src_port & mask->hdr.src_port)
 +if (mask->hdr.src_port)

Otherwise one can't match traffic where source/destination ports are 0. Yes
such traffic should be invalid, however that's precisely why one would want
to match it: drop before it reaches the protocol stack.

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-03  8:20     ` Adrien Mazarguil
@ 2018-08-05  6:10       ` Matan Azrad
  2018-08-06  9:40         ` Adrien Mazarguil
  0 siblings, 1 reply; 11+ messages in thread
From: Matan Azrad @ 2018-08-05  6:10 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: Keith Wiles, Ophir Munk, dev, stable

Hi Adrien

From: Adrien Mazarguil
> Hi Matan,
> 
> On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> > Hi Adrien
> >
> > From: Adrien Mazarguil
> > > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > > The rte_flow meaning of zero flow mask configuration is to match
> > > > all the range of the item value.
> > > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > > > should much all the ipv4 traffic from the rte_flow API perspective.
> > > >
> > > > From some kernel perspectives the above rule means to ignore all
> > > > the
> > > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > > >
> > > > Due to the fact that the tap PMD should provide the rte_flow
> > > > meaning, it is necessary to ignore the spec in case the mask is
> > > > zero when it forwards such like flows to the kernel.
> > > > So, the above rule should be translated to eth / ipv4 to get the
> > > > correct meaning.
> > > >
> > > > Ignore spec configurations when the mask is zero.
> > >
> > > I would go further, one should be able to match IP address 0.0.0.0 for
> instance.
> > > The PMD should only trust the mask on all fields without looking at spec.
> >
> > The PMD should convert the RTE flow API to the device configuration,
> > So I can think on scenarios that the PMD should look on spec.
> 
> Obviously the PMD needs to take spec into account. What I meant is that for
> each field, spec must be taken into account according to mask only.
> 
> For any given field, when mask is empty, don't look at spec, it's like a wildcard.
> When mask is full, take spec as is, even if spec only contains zeroed bits.
> 
> User intent in that case is to match a zero value exactly, so it must not result in
> a wildcard match. If supported, when mask is partial, masked bits are also
> matched exactly, even if these turn out to be a zero value. Unmasked bits are
> considered wildcards.
> 

Yes I understand your point Adrien, but I mean that maybe sometimes some spec values should be converted to another spec values to get the correct translation of rte_flow for a special device.

Here, maybe IP_spec=0.0.0.0 is a special case that should be taken into account, so we must validate what's happen in Tap for this case to apply your suggestion too, Maybe there was some intentions for spec=0 cases from the current code author.
 

> In short, to address both the issue mentioned in the commit log and the one I'm
> talking about, you only need to replace "spec" with "mask" in the original code.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-05  6:10       ` Matan Azrad
@ 2018-08-06  9:40         ` Adrien Mazarguil
  2018-08-06  9:58           ` Matan Azrad
  0 siblings, 1 reply; 11+ messages in thread
From: Adrien Mazarguil @ 2018-08-06  9:40 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Keith Wiles, Ophir Munk, dev, stable

On Sun, Aug 05, 2018 at 06:10:55AM +0000, Matan Azrad wrote:
> Hi Adrien
> 
> From: Adrien Mazarguil
> > Hi Matan,
> > 
> > On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> > > Hi Adrien
> > >
> > > From: Adrien Mazarguil
> > > > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > > > The rte_flow meaning of zero flow mask configuration is to match
> > > > > all the range of the item value.
> > > > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > > > > should much all the ipv4 traffic from the rte_flow API perspective.
> > > > >
> > > > > From some kernel perspectives the above rule means to ignore all
> > > > > the
> > > > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > > > >
> > > > > Due to the fact that the tap PMD should provide the rte_flow
> > > > > meaning, it is necessary to ignore the spec in case the mask is
> > > > > zero when it forwards such like flows to the kernel.
> > > > > So, the above rule should be translated to eth / ipv4 to get the
> > > > > correct meaning.
> > > > >
> > > > > Ignore spec configurations when the mask is zero.
> > > >
> > > > I would go further, one should be able to match IP address 0.0.0.0 for
> > instance.
> > > > The PMD should only trust the mask on all fields without looking at spec.
> > >
> > > The PMD should convert the RTE flow API to the device configuration,
> > > So I can think on scenarios that the PMD should look on spec.
> > 
> > Obviously the PMD needs to take spec into account. What I meant is that for
> > each field, spec must be taken into account according to mask only.
> > 
> > For any given field, when mask is empty, don't look at spec, it's like a wildcard.
> > When mask is full, take spec as is, even if spec only contains zeroed bits.
> > 
> > User intent in that case is to match a zero value exactly, so it must not result in
> > a wildcard match. If supported, when mask is partial, masked bits are also
> > matched exactly, even if these turn out to be a zero value. Unmasked bits are
> > considered wildcards.
> > 
> 
> Yes I understand your point Adrien, but I mean that maybe sometimes some spec values should be converted to another spec values to get the correct translation of rte_flow for a special device.
> 
> Here, maybe IP_spec=0.0.0.0 is a special case that should be taken into account, so we must validate what's happen in Tap for this case to apply your suggestion too, Maybe there was some intentions for spec=0 cases from the current code author.

I understand that's a lot of maybes :)

I've checked the code and I'am sure it's a mistake made by the original
author. See tap_flow_create_eth() for instance:

 if (!is_zero_ether_addr(&spec->dst)) {

Followed by:

 if (!is_zero_ether_addr(&mask->src))

This lack of consistency doesn't make any sense, it cannot be on purpose.

To my credentials I wrote a very similar code which uses TC flower in mlx5
and relies on mask (only) in order to retrieve spec. Have a look at
drivers/net/mlx5/mlx5_nl_flow.c. I validated that traffic where addresses
were all zeroes could be successfully matched.

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/tap: fix zeroed flow mask configurations
  2018-08-06  9:40         ` Adrien Mazarguil
@ 2018-08-06  9:58           ` Matan Azrad
  0 siblings, 0 replies; 11+ messages in thread
From: Matan Azrad @ 2018-08-06  9:58 UTC (permalink / raw)
  To: Adrien Mazarguil; +Cc: Keith Wiles, Ophir Munk, dev, stable

Hi Adrien

From: Adrien Mazarguil
> On Sun, Aug 05, 2018 at 06:10:55AM +0000, Matan Azrad wrote:
> > Hi Adrien
> >
> > From: Adrien Mazarguil
> > > Hi Matan,
> > >
> > > On Thu, Aug 02, 2018 at 05:52:18PM +0000, Matan Azrad wrote:
> > > > Hi Adrien
> > > >
> > > > From: Adrien Mazarguil
> > > > > On Thu, Aug 02, 2018 at 10:33:00AM +0000, Matan Azrad wrote:
> > > > > > The rte_flow meaning of zero flow mask configuration is to
> > > > > > match all the range of the item value.
> > > > > > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask
> > > > > > 0.0.0.0 should much all the ipv4 traffic from the rte_flow API
> perspective.
> > > > > >
> > > > > > From some kernel perspectives the above rule means to ignore
> > > > > > all the
> > > > > > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > > > > >
> > > > > > Due to the fact that the tap PMD should provide the rte_flow
> > > > > > meaning, it is necessary to ignore the spec in case the mask
> > > > > > is zero when it forwards such like flows to the kernel.
> > > > > > So, the above rule should be translated to eth / ipv4 to get
> > > > > > the correct meaning.
> > > > > >
> > > > > > Ignore spec configurations when the mask is zero.
> > > > >
> > > > > I would go further, one should be able to match IP address
> > > > > 0.0.0.0 for
> > > instance.
> > > > > The PMD should only trust the mask on all fields without looking at
> spec.
> > > >
> > > > The PMD should convert the RTE flow API to the device
> > > > configuration, So I can think on scenarios that the PMD should look on
> spec.
> > >
> > > Obviously the PMD needs to take spec into account. What I meant is
> > > that for each field, spec must be taken into account according to mask
> only.
> > >
> > > For any given field, when mask is empty, don't look at spec, it's like a
> wildcard.
> > > When mask is full, take spec as is, even if spec only contains zeroed bits.
> > >
> > > User intent in that case is to match a zero value exactly, so it
> > > must not result in a wildcard match. If supported, when mask is
> > > partial, masked bits are also matched exactly, even if these turn
> > > out to be a zero value. Unmasked bits are considered wildcards.
> > >
> >
> > Yes I understand your point Adrien, but I mean that maybe sometimes
> some spec values should be converted to another spec values to get the
> correct translation of rte_flow for a special device.
> >
> > Here, maybe IP_spec=0.0.0.0 is a special case that should be taken into
> account, so we must validate what's happen in Tap for this case to apply your
> suggestion too, Maybe there was some intentions for spec=0 cases from the
> current code author.
> 
> I understand that's a lot of maybes :)
> 
> I've checked the code and I'am sure it's a mistake made by the original
> author. See tap_flow_create_eth() for instance:
> 
>  if (!is_zero_ether_addr(&spec->dst)) {
> 
> Followed by:
> 
>  if (!is_zero_ether_addr(&mask->src))
> 
> This lack of consistency doesn't make any sense, it cannot be on purpose.
> 
> To my credentials I wrote a very similar code which uses TC flower in mlx5
> and relies on mask (only) in order to retrieve spec. Have a look at
> drivers/net/mlx5/mlx5_nl_flow.c. I validated that traffic where addresses
> were all zeroes could be successfully matched.
>

I will check the spec zero cases and will update.

Thanks Adrien!

 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-stable] [PATCH v2] net/tap: fix zeroed flow mask configurations
  2018-08-02 10:33 [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations Matan Azrad
  2018-08-02 12:03 ` Wiles, Keith
  2018-08-02 14:27 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil
@ 2018-08-06 10:58 ` Matan Azrad
  2018-08-06 13:16   ` Adrien Mazarguil
  2 siblings, 1 reply; 11+ messages in thread
From: Matan Azrad @ 2018-08-06 10:58 UTC (permalink / raw)
  To: Keith Wiles; +Cc: Ophir Munk, Adrien Mazarguil, dev, stable

The rte_flow meaning of zero flow mask configuration is to match all
the range of the item value.
For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
should much all the ipv4 traffic from the rte_flow API perspective.

>From some kernel perspectives the above rule means to ignore all the
ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).

Due to the fact that the tap PMD should provide the rte_flow meaning,
it is necessary to ignore the spec in case the mask is zero when it
forwards such like flows to the kernel.
So, the above rule should be translated to eth / ipv4 to get the
correct meaning.

Ignore spec configurations when the mask is zero.

Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

V2:
Address Adrien comments to fix also the spec=0 check.

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index 6b60e6d..0e01af6 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -537,7 +537,7 @@ tap_flow_create_eth(const struct rte_flow_item *item, void *data)
 	if (!flow)
 		return 0;
 	msg = &flow->msg;
-	if (!is_zero_ether_addr(&spec->dst)) {
+	if (!is_zero_ether_addr(&mask->dst)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_ETH_DST, ETHER_ADDR_LEN,
 			   &spec->dst.addr_bytes);
 		tap_nlattr_add(&msg->nh,
@@ -651,13 +651,13 @@ tap_flow_create_ipv4(const struct rte_flow_item *item, void *data)
 		info->eth_type = htons(ETH_P_IP);
 	if (!spec)
 		return 0;
-	if (spec->hdr.dst_addr) {
+	if (mask->hdr.dst_addr) {
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST,
 			     spec->hdr.dst_addr);
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_DST_MASK,
 			     mask->hdr.dst_addr);
 	}
-	if (spec->hdr.src_addr) {
+	if (mask->hdr.src_addr) {
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC,
 			     spec->hdr.src_addr);
 		tap_nlattr_add32(&msg->nh, TCA_FLOWER_KEY_IPV4_SRC_MASK,
@@ -707,13 +707,13 @@ tap_flow_create_ipv6(const struct rte_flow_item *item, void *data)
 		info->eth_type = htons(ETH_P_IPV6);
 	if (!spec)
 		return 0;
-	if (memcmp(spec->hdr.dst_addr, empty_addr, 16)) {
+	if (memcmp(mask->hdr.dst_addr, empty_addr, 16)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST,
 			   sizeof(spec->hdr.dst_addr), &spec->hdr.dst_addr);
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_DST_MASK,
 			   sizeof(mask->hdr.dst_addr), &mask->hdr.dst_addr);
 	}
-	if (memcmp(spec->hdr.src_addr, empty_addr, 16)) {
+	if (memcmp(mask->hdr.src_addr, empty_addr, 16)) {
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC,
 			   sizeof(spec->hdr.src_addr), &spec->hdr.src_addr);
 		tap_nlattr_add(&msg->nh, TCA_FLOWER_KEY_IPV6_SRC_MASK,
@@ -762,10 +762,10 @@ tap_flow_create_udp(const struct rte_flow_item *item, void *data)
 	tap_nlattr_add8(&msg->nh, TCA_FLOWER_KEY_IP_PROTO, IPPROTO_UDP);
 	if (!spec)
 		return 0;
-	if (spec->hdr.dst_port & mask->hdr.dst_port)
+	if (mask->hdr.dst_port)
 		tap_nlattr_add16(&msg->nh, TCA_FLOWER_KEY_UDP_DST,
 			     spec->hdr.dst_port);
-	if (spec->hdr.src_port & mask->hdr.src_port)
+	if (mask->hdr.src_port)
 		tap_nlattr_add16(&msg->nh, TCA_FLOWER_KEY_UDP_SRC,
 			     spec->hdr.src_port);
 	return 0;
@@ -808,10 +808,10 @@ tap_flow_create_tcp(const struct rte_flow_item *item, void *data)
 	tap_nlattr_add8(&msg->nh, TCA_FLOWER_KEY_IP_PROTO, IPPROTO_TCP);
 	if (!spec)
 		return 0;
-	if (spec->hdr.dst_port & mask->hdr.dst_port)
+	if (mask->hdr.dst_port)
 		tap_nlattr_add16(&msg->nh, TCA_FLOWER_KEY_TCP_DST,
 			     spec->hdr.dst_port);
-	if (spec->hdr.src_port & mask->hdr.src_port)
+	if (mask->hdr.src_port)
 		tap_nlattr_add16(&msg->nh, TCA_FLOWER_KEY_TCP_SRC,
 			     spec->hdr.src_port);
 	return 0;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/tap: fix zeroed flow mask configurations
  2018-08-06 10:58 ` [dpdk-stable] [PATCH v2] " Matan Azrad
@ 2018-08-06 13:16   ` Adrien Mazarguil
  2018-08-07 14:01     ` Thomas Monjalon
  0 siblings, 1 reply; 11+ messages in thread
From: Adrien Mazarguil @ 2018-08-06 13:16 UTC (permalink / raw)
  To: Matan Azrad; +Cc: Keith Wiles, Ophir Munk, dev, stable

On Mon, Aug 06, 2018 at 10:58:47AM +0000, Matan Azrad wrote:
> The rte_flow meaning of zero flow mask configuration is to match all
> the range of the item value.
> For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> should much all the ipv4 traffic from the rte_flow API perspective.
> 
> From some kernel perspectives the above rule means to ignore all the
> ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> 
> Due to the fact that the tap PMD should provide the rte_flow meaning,
> it is necessary to ignore the spec in case the mask is zero when it
> forwards such like flows to the kernel.
> So, the above rule should be translated to eth / ipv4 to get the
> correct meaning.
> 
> Ignore spec configurations when the mask is zero.
> 
> Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/tap/tap_flow.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> V2:
> Address Adrien comments to fix also the spec=0 check.

Thanks,

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-stable] [PATCH v2] net/tap: fix zeroed flow mask configurations
  2018-08-06 13:16   ` Adrien Mazarguil
@ 2018-08-07 14:01     ` Thomas Monjalon
  0 siblings, 0 replies; 11+ messages in thread
From: Thomas Monjalon @ 2018-08-07 14:01 UTC (permalink / raw)
  To: Matan Azrad; +Cc: stable, Adrien Mazarguil, Keith Wiles, Ophir Munk, dev

06/08/2018 15:16, Adrien Mazarguil:
> On Mon, Aug 06, 2018 at 10:58:47AM +0000, Matan Azrad wrote:
> > The rte_flow meaning of zero flow mask configuration is to match all
> > the range of the item value.
> > For example, the flow eth / ipv4 dst spec 1.2.3.4 dst mask 0.0.0.0
> > should much all the ipv4 traffic from the rte_flow API perspective.
> > 
> > From some kernel perspectives the above rule means to ignore all the
> > ipv4 traffic (e.g. Ubuntu 16.04, 4.15.10).
> > 
> > Due to the fact that the tap PMD should provide the rte_flow meaning,
> > it is necessary to ignore the spec in case the mask is zero when it
> > forwards such like flows to the kernel.
> > So, the above rule should be translated to eth / ipv4 to get the
> > correct meaning.
> > 
> > Ignore spec configurations when the mask is zero.
> > 
> > Fixes: de96fe68ae95 ("net/tap: add basic flow API patterns and actions")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  drivers/net/tap/tap_flow.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > V2:
> > Address Adrien comments to fix also the spec=0 check.
> 
> Thanks,
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-08-07 14:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 10:33 [dpdk-stable] [PATCH] net/tap: fix zeroed flow mask configurations Matan Azrad
2018-08-02 12:03 ` Wiles, Keith
2018-08-02 14:27 ` [dpdk-stable] [dpdk-dev] " Adrien Mazarguil
2018-08-02 17:52   ` Matan Azrad
2018-08-03  8:20     ` Adrien Mazarguil
2018-08-05  6:10       ` Matan Azrad
2018-08-06  9:40         ` Adrien Mazarguil
2018-08-06  9:58           ` Matan Azrad
2018-08-06 10:58 ` [dpdk-stable] [PATCH v2] " Matan Azrad
2018-08-06 13:16   ` Adrien Mazarguil
2018-08-07 14:01     ` Thomas Monjalon

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).