1. Patchew
  2. linux
  3. View series

[PATCH net] ice: fix invalid check for empty list in ice_sched_assoc_vsi_to_agg()

Jakob Koschel posted 1 patch 2 years, 11 months ago
There is a newer version of this series
drivers/net/ethernet/intel/ice/ice_sched.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
[PATCH net] ice: fix invalid check for empty list in ice_sched_assoc_vsi_to_agg()
Posted by Jakob Koschel 2 years, 11 months ago 
The code implicitly assumes that the list iterator finds a correct
handle. If 'vsi_handle' is not found the 'old_agg_vsi_info' was
pointing to an bogus memory location. For safety a separate list
iterator variable should be used to make the != NULL check on
'old_agg_vsi_info' correct under any circumstances.

Additionally Linus proposed to avoid any use of the list iterator
variable after the loop, in the attempt to move the list iterator
variable declaration into the macro to avoid any potential misuse after
the loop. Using it in a pointer comparision after the loop is undefined
behavior and should be omitted if possible [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jkl820.git@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice_sched.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index 4eca8d195ef0..b7682de0ae05 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -2788,7 +2788,7 @@ static int
 ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 			   u16 vsi_handle, unsigned long *tc_bitmap)
 {
-	struct ice_sched_agg_vsi_info *agg_vsi_info, *old_agg_vsi_info = NULL;
+	struct ice_sched_agg_vsi_info *agg_vsi_info, *iter, *old_agg_vsi_info = NULL;
 	struct ice_sched_agg_info *agg_info, *old_agg_info;
 	struct ice_hw *hw = pi->hw;
 	int status = 0;
@@ -2806,11 +2806,13 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
 	if (old_agg_info && old_agg_info != agg_info) {
 		struct ice_sched_agg_vsi_info *vtmp;
 
-		list_for_each_entry_safe(old_agg_vsi_info, vtmp,
+		list_for_each_entry_safe(iter, vtmp,
 					 &old_agg_info->agg_vsi_list,
 					 list_entry)
-			if (old_agg_vsi_info->vsi_handle == vsi_handle)
+			if (iter->vsi_handle == vsi_handle) {
+				old_agg_vsi_info = iter;
 				break;
+			}
 	}
 
 	/* check if entry already exist */

---
base-commit: eeac8ede17557680855031c6f305ece2378af326
change-id: 20230301-ice-fix-invalid-iterator-found-check-0a3e5b43dfb3

Best regards,
-- 
Jakob Koschel <jkl820.git@gmail.com>
Re: [PATCH net] ice: fix invalid check for empty list in ice_sched_assoc_vsi_to_agg()
Posted by Tony Nguyen 2 years, 11 months ago 
On 3/13/2023 9:31 AM, Jakob Koschel wrote:
> The code implicitly assumes that the list iterator finds a correct
> handle. If 'vsi_handle' is not found the 'old_agg_vsi_info' was
> pointing to an bogus memory location. For safety a separate list
> iterator variable should be used to make the != NULL check on
> 'old_agg_vsi_info' correct under any circumstances.
> 
> Additionally Linus proposed to avoid any use of the list iterator
> variable after the loop, in the attempt to move the list iterator
> variable declaration into the macro to avoid any potential misuse after
> the loop. Using it in a pointer comparision after the loop is undefined
> behavior and should be omitted if possible [1].
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jkl820.git@gmail.com>

The patch looks fine, however, for net patches, could you please include 
a Fixes: tag.

Thanks,
Tony

> ---
>   drivers/net/ethernet/intel/ice/ice_sched.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index 4eca8d195ef0..b7682de0ae05 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> @@ -2788,7 +2788,7 @@ static int
>   ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>   			   u16 vsi_handle, unsigned long *tc_bitmap)
>   {
> -	struct ice_sched_agg_vsi_info *agg_vsi_info, *old_agg_vsi_info = NULL;
> +	struct ice_sched_agg_vsi_info *agg_vsi_info, *iter, *old_agg_vsi_info = NULL;
>   	struct ice_sched_agg_info *agg_info, *old_agg_info;
>   	struct ice_hw *hw = pi->hw;
>   	int status = 0;
> @@ -2806,11 +2806,13 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>   	if (old_agg_info && old_agg_info != agg_info) {
>   		struct ice_sched_agg_vsi_info *vtmp;
>   
> -		list_for_each_entry_safe(old_agg_vsi_info, vtmp,
> +		list_for_each_entry_safe(iter, vtmp,
>   					 &old_agg_info->agg_vsi_list,
>   					 list_entry)
> -			if (old_agg_vsi_info->vsi_handle == vsi_handle)
> +			if (iter->vsi_handle == vsi_handle) {
> +				old_agg_vsi_info = iter;
>   				break;
> +			}
>   	}
>   
>   	/* check if entry already exist */
> 
> ---
> base-commit: eeac8ede17557680855031c6f305ece2378af326
> change-id: 20230301-ice-fix-invalid-iterator-found-check-0a3e5b43dfb3
> 
> Best regards,
Re: [Intel-wired-lan] [PATCH net] ice: fix invalid check for empty list in ice_sched_assoc_vsi_to_agg()
Posted by Paul Menzel 2 years, 11 months ago 
Dear Jakob,


Thank you for the patch.

Am 13.03.23 um 17:31 schrieb Jakob Koschel:
> The code implicitly assumes that the list iterator finds a correct
> handle. If 'vsi_handle' is not found the 'old_agg_vsi_info' was
> pointing to an bogus memory location. For safety a separate list
> iterator variable should be used to make the != NULL check on
> 'old_agg_vsi_info' correct under any circumstances.
> 
> Additionally Linus proposed to avoid any use of the list iterator
> variable after the loop, in the attempt to move the list iterator
> variable declaration into the macro to avoid any potential misuse after
> the loop. Using it in a pointer comparision after the loop is undefined

compar*i*son

> behavior and should be omitted if possible [1].

(It took me a short time to find the reference number at the end of the 
URL.)

> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jkl820.git@gmail.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_sched.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index 4eca8d195ef0..b7682de0ae05 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> @@ -2788,7 +2788,7 @@ static int
>   ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>   			   u16 vsi_handle, unsigned long *tc_bitmap)
>   {
> -	struct ice_sched_agg_vsi_info *agg_vsi_info, *old_agg_vsi_info = NULL;
> +	struct ice_sched_agg_vsi_info *agg_vsi_info, *iter, *old_agg_vsi_info = NULL;
>   	struct ice_sched_agg_info *agg_info, *old_agg_info;
>   	struct ice_hw *hw = pi->hw;
>   	int status = 0;
> @@ -2806,11 +2806,13 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>   	if (old_agg_info && old_agg_info != agg_info) {
>   		struct ice_sched_agg_vsi_info *vtmp;
>   
> -		list_for_each_entry_safe(old_agg_vsi_info, vtmp,
> +		list_for_each_entry_safe(iter, vtmp,
>   					 &old_agg_info->agg_vsi_list,
>   					 list_entry)
> -			if (old_agg_vsi_info->vsi_handle == vsi_handle)
> +			if (iter->vsi_handle == vsi_handle) {
> +				old_agg_vsi_info = iter;
>   				break;
> +			}
>   	}
>   
>   	/* check if entry already exist */

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.