[PATCH net 0/2] net/sched: cls_flower: Fix mask handling

[PATCH net 0/2] net/sched: cls_flower: Fix mask handling

[Paul Blakey]

Hi,
The series fix how the mask is being handled.
Thanks.

Paul Blakey (2):
  net/sched: cls_flower: Use mask for addr_type
  net/sched: cls_flower: Use masked key when calling HW offloads

 net/sched/cls_flower.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH net 1/2] net/sched: cls_flower: Use mask for addr_type

[Paul Blakey]

When addr_type is set, mask should also be set.

Fixes: 66530bdf85eb ('sched,cls_flower: set key address type when present')
Fixes: bc3103f1ed40 ('net/sched: cls_flower: Classify packet in ip tunnels')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_flower.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e040c51..9758f5a 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -509,6 +509,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 
 	if (tb[TCA_FLOWER_KEY_IPV4_SRC] || tb[TCA_FLOWER_KEY_IPV4_DST]) {
 		key->control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
+		mask->control.addr_type = ~0;
 		fl_set_key_val(tb, &key->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC,
 			       &mask->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC_MASK,
 			       sizeof(key->ipv4.src));
@@ -517,6 +518,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       sizeof(key->ipv4.dst));
 	} else if (tb[TCA_FLOWER_KEY_IPV6_SRC] || tb[TCA_FLOWER_KEY_IPV6_DST]) {
 		key->control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
+		mask->control.addr_type = ~0;
 		fl_set_key_val(tb, &key->ipv6.src, TCA_FLOWER_KEY_IPV6_SRC,
 			       &mask->ipv6.src, TCA_FLOWER_KEY_IPV6_SRC_MASK,
 			       sizeof(key->ipv6.src));
@@ -571,6 +573,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
 	    tb[TCA_FLOWER_KEY_ENC_IPV4_DST]) {
 		key->enc_control.addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
+		mask->enc_control.addr_type = ~0;
 		fl_set_key_val(tb, &key->enc_ipv4.src,
 			       TCA_FLOWER_KEY_ENC_IPV4_SRC,
 			       &mask->enc_ipv4.src,
@@ -586,6 +589,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 	if (tb[TCA_FLOWER_KEY_ENC_IPV6_SRC] ||
 	    tb[TCA_FLOWER_KEY_ENC_IPV6_DST]) {
 		key->enc_control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
+		mask->enc_control.addr_type = ~0;
 		fl_set_key_val(tb, &key->enc_ipv6.src,
 			       TCA_FLOWER_KEY_ENC_IPV6_SRC,
 			       &mask->enc_ipv6.src,
-- 
1.8.3.1

[PATCH net 2/2] net/sched: cls_flower: Use masked key when calling HW offloads

[Paul Blakey]

Zero bits on the mask signify a "don't care" on the corresponding bits
in key. Some HWs require those bits on the key to be zero. Since these
bits are masked anyway, it's okay to provide the masked key to all
drivers.

Fixes: 5b33f48842fa ('net/flower: Introduce hardware offload support')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_flower.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 9758f5a..35ac28d 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -252,7 +252,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
 	offload.cookie = (unsigned long)f;
 	offload.dissector = dissector;
 	offload.mask = mask;
-	offload.key = &f->key;
+	offload.key = &f->mkey;
 	offload.exts = &f->exts;
 
 	tc->type = TC_SETUP_CLSFLOWER;
-- 
1.8.3.1

Re: [PATCH net 2/2] net/sched: cls_flower: Use masked key when calling HW offloads

[Simon Horman]

Hi Paul,

On Wed, Dec 14, 2016 at 07:00:58PM +0200, Paul Blakey wrote:
> Zero bits on the mask signify a "don't care" on the corresponding bits
> in key. Some HWs require those bits on the key to be zero. Since these
> bits are masked anyway, it's okay to provide the masked key to all
> drivers.
> 
> Fixes: 5b33f48842fa ('net/flower: Introduce hardware offload support')
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>

While I don't have a specific use case in mind that this change would break
it seems to me that it would be better to handle hardware requirements
at the driver level.

> ---
>  net/sched/cls_flower.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 9758f5a..35ac28d 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -252,7 +252,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
>  	offload.cookie = (unsigned long)f;
>  	offload.dissector = dissector;
>  	offload.mask = mask;
> -	offload.key = &f->key;
> +	offload.key = &f->mkey;
>  	offload.exts = &f->exts;
>  
>  	tc->type = TC_SETUP_CLSFLOWER;
> -- 
> 1.8.3.1
> 

Re: [PATCH net 2/2] net/sched: cls_flower: Use masked key when calling HW offloads

[Jiri Pirko]

Thu, Dec 15, 2016 at 02:50:44PM CET, simon.horman@netronome.com wrote:
>Hi Paul,
>
>On Wed, Dec 14, 2016 at 07:00:58PM +0200, Paul Blakey wrote:
>> Zero bits on the mask signify a "don't care" on the corresponding bits
>> in key. Some HWs require those bits on the key to be zero. Since these
>> bits are masked anyway, it's okay to provide the masked key to all
>> drivers.
>> 
>> Fixes: 5b33f48842fa ('net/flower: Introduce hardware offload support')
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>
>While I don't have a specific use case in mind that this change would break
>it seems to me that it would be better to handle hardware requirements
>at the driver level.

Even though, makes no sense to pass unmasked key down. Is is only
confusing. This patch fixes it.


>
>> ---
>>  net/sched/cls_flower.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index 9758f5a..35ac28d 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -252,7 +252,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
>>  	offload.cookie = (unsigned long)f;
>>  	offload.dissector = dissector;
>>  	offload.mask = mask;
>> -	offload.key = &f->key;
>> +	offload.key = &f->mkey;
>>  	offload.exts = &f->exts;
>>  
>>  	tc->type = TC_SETUP_CLSFLOWER;
>> -- 
>> 1.8.3.1
>> 

Re: [PATCH net 2/2] net/sched: cls_flower: Use masked key when calling HW offloads

[Simon Horman]

On Thu, Dec 15, 2016 at 04:12:05PM +0200, Or Gerlitz wrote:
> On 12/15/2016 3:50 PM, Simon Horman wrote:
> >>Zero bits on the mask signify a "don't care" on the corresponding bits
> >>in key. Some HWs require those bits on the key to be zero. Since these
> >>bits are masked anyway, it's okay to provide the masked key to all
> >>drivers.
> >>
> >>Fixes: 5b33f48842fa ('net/flower: Introduce hardware offload support')
> >>
> >While I don't have a specific use case in mind that this change would break
> >it seems to me that it would be better to handle hardware requirements
> >at the driver level.
> 
> Simon, again, since these bits are masked anyway, it would be correct to
> provide the masked key to the hw device.
> 
> E.g no matter if the flow key/mask provided to the HW device is is
> 1.1.1.10/24  or 1.1.1.0/24, the user expects to the same matching, so
> nothing can't happen if we provide the latter to the driver.

> >While I don't have a specific use case in mind that this change would break
> >it seems to me that it would be better to handle hardware requirements
> >at the driver level.
> 
> Even though, makes no sense to pass unmasked key down. Is is only
> confusing. This patch fixes it.

It seems somewhat arbitrary to me to allow such filters in software
but not pass then down to the driver layer. But I don't feel strongly
about this and I am happy for the patch to progress as-is.

Re: [PATCH net 0/2] net/sched: cls_flower: Fix mask handling

[David Miller]

From: Paul Blakey <paulb@mellanox.com>
Date: Wed, 14 Dec 2016 19:00:56 +0200

> The series fix how the mask is being handled.

I guess this is reasonable, series applied, thanks.