diff mbox series

[net,v3] i40e: Disallow ip4 and ip6 l4_4_bytes

Message ID 20221115084925.2489227-1-kamil.maziarz@intel.com
State Accepted
Delegated to: Anthony Nguyen
Headers show
Series [net,v3] i40e: Disallow ip4 and ip6 l4_4_bytes | expand

Commit Message

Kamil Maziarz Nov. 15, 2022, 8:49 a.m. UTC 
From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>

Return -EOPNOTSUPP, when user requests l4_4_bytes for raw IP4 or
IP6 flow director filters. Flow director does not support filtering
on l4 bytes for PCTYPEs used by IP4 and IP6 filters.
Without this patch, user could create filters with l4_4_bytes fields,
which did not do any filtering on L4, but only on L3 fields.

Fixes: 36777d9fa24c ("i40e: check current configured input set when adding ntuple filters")
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Signed-off-by: Kamil Maziarz  <kamil.maziarz@intel.com>
---
 v3: removed footer and added Fixes tag
---
 v2: changed author and tree
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

Comments

Paul Menzel Nov. 15, 2022, 9:56 a.m. UTC | #1 
[Cc: +Jacob]

Dear Przemyslaw, dear Kamil,


Am 15.11.22 um 09:49 schrieb Kamil Maziarz:
> From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
> 
> Return -EOPNOTSUPP, when user requests l4_4_bytes for raw IP4 or
> IP6 flow director filters. Flow director does not support filtering
> on l4 bytes for PCTYPEs used by IP4 and IP6 filters.
> Without this patch, user could create filters with l4_4_bytes fields,
> which did not do any filtering on L4, but only on L3 fields.
> 
> Fixes: 36777d9fa24c ("i40e: check current configured input set when adding ntuple filters")

Are you sure that is the correct commit. It only seems to have 
refactored stuff, …

> Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
> Signed-off-by: Kamil Maziarz  <kamil.maziarz@intel.com>
> ---
>   v3: removed footer and added Fixes tag
> ---
>   v2: changed author and tree
> ---
>   drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 156e92c43780..6695dbe61a04 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -4447,11 +4447,7 @@ static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
>   			return -EOPNOTSUPP;
>   
>   		/* First 4 bytes of L4 header */
> -		if (usr_ip4_spec->l4_4_bytes == htonl(0xFFFFFFFF))
> -			new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
> -		else if (!usr_ip4_spec->l4_4_bytes)
> -			new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
> -		else
> +		if (usr_ip4_spec->l4_4_bytes)
>   			return -EOPNOTSUPP;

and the condition before was

     if (!tcp_ip4_spec->ip4dst || ~tcp_ip4_spec->ip4dst)

>   
>   		/* Filtering on Type of Service is not supported. */
> @@ -4490,11 +4486,7 @@ static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
>   		else
>   			return -EOPNOTSUPP;
>   
> -		if (usr_ip6_spec->l4_4_bytes == htonl(0xFFFFFFFF))
> -			new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
> -		else if (!usr_ip6_spec->l4_4_bytes)
> -			new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
> -		else
> +		if (usr_ip6_spec->l4_4_bytes)
>   			return -EOPNOTSUPP;
>   
>   		/* Filtering on Traffic class is not supported. */


Kind regards,

Paul
Paul Menzel Nov. 15, 2022, 2:31 p.m. UTC | #2 
[To: - <przemyslawx.patynowski@intel.com>, undeliverable]
[Cc: +Jacob, for real this time]

Am 15.11.22 um 10:56 schrieb Paul Menzel:
> [Cc: +Jacob]
> 
> Dear Przemyslaw, dear Kamil,
> 
> 
> Am 15.11.22 um 09:49 schrieb Kamil Maziarz:
>> From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>>
>> Return -EOPNOTSUPP, when user requests l4_4_bytes for raw IP4 or
>> IP6 flow director filters. Flow director does not support filtering
>> on l4 bytes for PCTYPEs used by IP4 and IP6 filters.
>> Without this patch, user could create filters with l4_4_bytes fields,
>> which did not do any filtering on L4, but only on L3 fields.
>>
>> Fixes: 36777d9fa24c ("i40e: check current configured input set when 
>> adding ntuple filters")
> 
> Are you sure that is the correct commit. It only seems to have 
> refactored stuff, …
> 
>> Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>> Signed-off-by: Kamil Maziarz  <kamil.maziarz@intel.com>
>> ---
>>   v3: removed footer and added Fixes tag
>> ---
>>   v2: changed author and tree
>> ---
>>   drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 12 ++----------
>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c 
>> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> index 156e92c43780..6695dbe61a04 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> @@ -4447,11 +4447,7 @@ static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
>>               return -EOPNOTSUPP;
>>           /* First 4 bytes of L4 header */
>> -        if (usr_ip4_spec->l4_4_bytes == htonl(0xFFFFFFFF))
>> -            new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
>> -        else if (!usr_ip4_spec->l4_4_bytes)
>> -            new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
>> -        else
>> +        if (usr_ip4_spec->l4_4_bytes)
>>               return -EOPNOTSUPP;
> 
> and the condition before was
> 
>      if (!tcp_ip4_spec->ip4dst || ~tcp_ip4_spec->ip4dst)
> 
>>           /* Filtering on Type of Service is not supported. */
>> @@ -4490,11 +4486,7 @@ static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
>>           else
>>               return -EOPNOTSUPP;
>> -        if (usr_ip6_spec->l4_4_bytes == htonl(0xFFFFFFFF))
>> -            new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
>> -        else if (!usr_ip6_spec->l4_4_bytes)
>> -            new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
>> -        else
>> +        if (usr_ip6_spec->l4_4_bytes)
>>               return -EOPNOTSUPP;
>>           /* Filtering on Traffic class is not supported. */
> 
> 
> Kind regards,
> 
> Paul
Jacob Keller Nov. 15, 2022, 6:55 p.m. UTC | #3 
On 11/15/2022 6:31 AM, Paul Menzel wrote:
> [To: - <przemyslawx.patynowski@intel.com>, undeliverable]
> [Cc: +Jacob, for real this time]
> 
> Am 15.11.22 um 10:56 schrieb Paul Menzel:
>> [Cc: +Jacob]
>>
>> Dear Przemyslaw, dear Kamil,
>>
>>
>> Am 15.11.22 um 09:49 schrieb Kamil Maziarz:
>>> From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>>>
>>> Return -EOPNOTSUPP, when user requests l4_4_bytes for raw IP4 or
>>> IP6 flow director filters. Flow director does not support filtering
>>> on l4 bytes for PCTYPEs used by IP4 and IP6 filters.
>>> Without this patch, user could create filters with l4_4_bytes fields,
>>> which did not do any filtering on L4, but only on L3 fields.
>>>
>>> Fixes: 36777d9fa24c ("i40e: check current configured input set when 
>>> adding ntuple filters")
>>
>> Are you sure that is the correct commit. It only seems to have 
>> refactored stuff, …
>>

This changes the i40e_check_fdir_input_set to reject any flags when 
l4_4_bytes are set. That would definitely make the driver reject such 
filters.

>>> Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>>> Signed-off-by: Kamil Maziarz  <kamil.maziarz@intel.com>
>>> ---
>>>   v3: removed footer and added Fixes tag
>>> ---
>>>   v2: changed author and tree
>>> ---
>>>   drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 12 ++----------
>>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c 
>>> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>>> index 156e92c43780..6695dbe61a04 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>>> @@ -4447,11 +4447,7 @@ static int i40e_check_fdir_input_set(struct 
>>> i40e_vsi *vsi,
>>>               return -EOPNOTSUPP;
>>>           /* First 4 bytes of L4 header */
>>> -        if (usr_ip4_spec->l4_4_bytes == htonl(0xFFFFFFFF))
>>> -            new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
>>> -        else if (!usr_ip4_spec->l4_4_bytes)
>>> -            new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
>>> -        else
>>> +        if (usr_ip4_spec->l4_4_bytes)
>>>               return -EOPNOTSUPP;

Well the previous code here checks some values but it checks:

if its all FFs, or if its all zeros we accept it. But otherwise we 
should be rejecting the filter? I think thats correct...

Are you suggesting that checking for all 1s is not valid? Can you point 
to a specific example of what went wrong? I guess that its because the 
PTYPES used by raw IP4 don't allow L4 matching. Ok.

So now we just reject all non-zero values and we don't need to modify 
new_mask because for IP_USER_FLOW it will always start out as having L4 
fields cleared.


>>
>> and the condition before was
>>
>>      if (!tcp_ip4_spec->ip4dst || ~tcp_ip4_spec->ip4dst)
>>
>>>           /* Filtering on Type of Service is not supported. */
>>> @@ -4490,11 +4486,7 @@ static int i40e_check_fdir_input_set(struct 
>>> i40e_vsi *vsi,
>>>           else
>>>               return -EOPNOTSUPP;
>>> -        if (usr_ip6_spec->l4_4_bytes == htonl(0xFFFFFFFF))
>>> -            new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
>>> -        else if (!usr_ip6_spec->l4_4_bytes)
>>> -            new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
>>> -        else
>>> +        if (usr_ip6_spec->l4_4_bytes)
>>>               return -EOPNOTSUPP;
>>>           /* Filtering on Traffic class is not supported. */
>>
>>

I think this is is correct, it makes us begin rejecting any filters 
which set l4_4_bytes by essentially rejecting when the l4 fields are 
enabled (all 1s). We never supported partial masks.

This is correct as it stops the L4 fields in the IP_USER case for IPv4 
and IPv6.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>> Kind regards,
>>
>> Paul
G, GurucharanX Nov. 22, 2022, 12:59 p.m. UTC | #4 
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Kamil Maziarz
> Sent: Tuesday, November 15, 2022 2:19 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>; Maziarz,
> Kamil <kamil.maziarz@intel.com>
> Subject: [Intel-wired-lan] [PATCH net v3] i40e: Disallow ip4 and ip6
> l4_4_bytes
> 
> From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
> 
> Return -EOPNOTSUPP, when user requests l4_4_bytes for raw IP4 or
> IP6 flow director filters. Flow director does not support filtering on l4 bytes
> for PCTYPEs used by IP4 and IP6 filters.
> Without this patch, user could create filters with l4_4_bytes fields, which did
> not do any filtering on L4, but only on L3 fields.
> 
> Fixes: 36777d9fa24c ("i40e: check current configured input set when adding
> ntuple filters")
> Signed-off-by: Przemyslaw Patynowski
> <przemyslawx.patynowski@intel.com>
> Signed-off-by: Kamil Maziarz  <kamil.maziarz@intel.com>
> ---
>  v3: removed footer and added Fixes tag
> ---
>  v2: changed author and tree
> ---
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 

Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 156e92c43780..6695dbe61a04 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -4447,11 +4447,7 @@  static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
 			return -EOPNOTSUPP;
 
 		/* First 4 bytes of L4 header */
-		if (usr_ip4_spec->l4_4_bytes == htonl(0xFFFFFFFF))
-			new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
-		else if (!usr_ip4_spec->l4_4_bytes)
-			new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
-		else
+		if (usr_ip4_spec->l4_4_bytes)
 			return -EOPNOTSUPP;
 
 		/* Filtering on Type of Service is not supported. */
@@ -4490,11 +4486,7 @@  static int i40e_check_fdir_input_set(struct i40e_vsi *vsi,
 		else
 			return -EOPNOTSUPP;
 
-		if (usr_ip6_spec->l4_4_bytes == htonl(0xFFFFFFFF))
-			new_mask |= I40E_L4_SRC_MASK | I40E_L4_DST_MASK;
-		else if (!usr_ip6_spec->l4_4_bytes)
-			new_mask &= ~(I40E_L4_SRC_MASK | I40E_L4_DST_MASK);
-		else
+		if (usr_ip6_spec->l4_4_bytes)
 			return -EOPNOTSUPP;
 
 		/* Filtering on Traffic class is not supported. */