The bugs are here: if (old_agg_vsi_info) if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
The list iterator value 'old_agg_vsi_info' will *always* be set and non-NULL by list_for_each_entry_safe(), so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element found (in this case, the check 'if (old_agg_vsi_info)' will always be true unexpectly).
To fix the bug, use a new variable 'iter' as the list iterator, while use the original variable 'old_agg_vsi_info' as a dedicated pointer to point to the found element.
Cc: stable@vger.kernel.org Fixes: 37c592062b16d ("ice: remove the VSI info from previous agg") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/net/ethernet/intel/ice/ice_sched.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c index 7947223536e3..fba524148a09 100644 --- a/drivers/net/ethernet/intel/ice/ice_sched.c +++ b/drivers/net/ethernet/intel/ice/ice_sched.c @@ -2757,6 +2757,7 @@ 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 *iter; struct ice_sched_agg_info *agg_info, *old_agg_info; struct ice_hw *hw = pi->hw; int status = 0; @@ -2774,11 +2775,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 */
-----Original Message----- From: Xiaomeng Tong xiam0nd.tong@gmail.com Sent: Saturday, March 26, 2022 11:44 PM To: Brandeburg, Jesse jesse.brandeburg@intel.com Cc: Nguyen, Anthony L anthony.l.nguyen@intel.com; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Raj, Victor victor.raj@intel.com; intel- wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- kernel@vger.kernel.org; Xiaomeng Tong xiam0nd.tong@gmail.com; stable@vger.kernel.org Subject: [PATCH] ice: ice_sched: fix an incorrect NULL check on list iterator
The bugs are here: if (old_agg_vsi_info) if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
The list iterator value 'old_agg_vsi_info' will *always* be set and non-NULL by list_for_each_entry_safe(), so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element found (in this case, the check 'if (old_agg_vsi_info)' will always be true unexpectly).
To fix the bug, use a new variable 'iter' as the list iterator, while use the original variable 'old_agg_vsi_info' as a dedicated pointer to point to the found element.
Yep. This looks correct to me.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Thanks, Jake
Cc: stable@vger.kernel.org Fixes: 37c592062b16d ("ice: remove the VSI info from previous agg") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
drivers/net/ethernet/intel/ice/ice_sched.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c index 7947223536e3..fba524148a09 100644 --- a/drivers/net/ethernet/intel/ice/ice_sched.c +++ b/drivers/net/ethernet/intel/ice/ice_sched.c @@ -2757,6 +2757,7 @@ 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 *iter; struct ice_sched_agg_info *agg_info, *old_agg_info; struct ice_hw *hw = pi->hw; int status = 0;
@@ -2774,11 +2775,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 */
-- 2.17.1
-----Original Message-----
From: Xiaomeng Tong xiam0nd.tong@gmail.com Sent: Saturday, March 26, 2022 11:44 PM To: Brandeburg, Jesse jesse.brandeburg@intel.com Cc: Nguyen, Anthony L anthony.l.nguyen@intel.com; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Raj, Victor victor.raj@intel.com; intel- wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- kernel@vger.kernel.org; Xiaomeng Tong xiam0nd.tong@gmail.com; stable@vger.kernel.org Subject: [PATCH] ice: ice_sched: fix an incorrect NULL check on list iterator
The bugs are here: if (old_agg_vsi_info) if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
The list iterator value 'old_agg_vsi_info' will *always* be set and non-NULL by list_for_each_entry_safe(), so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element found (in this case, the check 'if (old_agg_vsi_info)' will always be true unexpectly).
To fix the bug, use a new variable 'iter' as the list iterator, while use the original variable 'old_agg_vsi_info' as a dedicated pointer to point to the found element.
Yep. This looks correct to me.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Thanks, Jake
Cc: stable@vger.kernel.org Fixes: 37c592062b16d ("ice: remove the VSI info from previous agg") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
drivers/net/ethernet/intel/ice/ice_sched.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Tested-by: Gurucharan gurucharanx.g@intel.com (A Contingent worker at Intel)
If the control comes down to the loop then surely the VSI is already part of an aggregator (old). The aggregator vsi list should have that VSI information. My understanding is that if control comes here then you always find a valid entry and matching handle here. If that doesn't happen then we need to debug. The fix is kind of masking this problem.
-Victor
-----Original Message----- From: G, GurucharanX gurucharanx.g@intel.com Sent: Monday, May 2, 2022 1:18 AM To: Keller, Jacob E jacob.e.keller@intel.com; Xiaomeng Tong xiam0nd.tong@gmail.com; Brandeburg, Jesse jesse.brandeburg@intel.com Cc: netdev@vger.kernel.org; Raj, Victor victor.raj@intel.com; stable@vger.kernel.org; linux-kernel@vger.kernel.org; intel-wired-lan@lists.osuosl.org; kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net Subject: RE: [PATCH] ice: ice_sched: fix an incorrect NULL check on list iterator
-----Original Message-----
From: Xiaomeng Tong xiam0nd.tong@gmail.com Sent: Saturday, March 26, 2022 11:44 PM To: Brandeburg, Jesse jesse.brandeburg@intel.com Cc: Nguyen, Anthony L anthony.l.nguyen@intel.com; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Raj, Victor victor.raj@intel.com; intel- wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux- kernel@vger.kernel.org; Xiaomeng Tong xiam0nd.tong@gmail.com; stable@vger.kernel.org Subject: [PATCH] ice: ice_sched: fix an incorrect NULL check on list iterator
The bugs are here: if (old_agg_vsi_info) if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
The list iterator value 'old_agg_vsi_info' will *always* be set and non-NULL by list_for_each_entry_safe(), so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element found (in this case, the check 'if (old_agg_vsi_info)' will always be true unexpectly).
To fix the bug, use a new variable 'iter' as the list iterator, while use the original variable 'old_agg_vsi_info' as a dedicated pointer to point to the found element.
Yep. This looks correct to me.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Thanks, Jake
Cc: stable@vger.kernel.org Fixes: 37c592062b16d ("ice: remove the VSI info from previous agg") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
drivers/net/ethernet/intel/ice/ice_sched.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Tested-by: Gurucharan gurucharanx.g@intel.com (A Contingent worker at Intel)
linux-stable-mirror@lists.linaro.org
-
G, GurucharanX -
Keller, Jacob E -
Raj, Victor -
Xiaomeng Tong