From: Bart Van Assche <bvanassche@acm.org>

Move the mutex_lock() call up to prevent that DCB settings change after
the ice_query_port_ets() call.

This patch fixes a bug in an error path. Without this patch pf->tc_mutex
may be unlocked in an error path without having been locked. This bug has
been detected by the clang thread-safety analyzer.

Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: intel-wired-lan@lists.osuosl.org
Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index bd77f1c001ee..78ded6876581 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -537,14 +537,14 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	struct ice_dcbx_cfg *err_cfg;
 	int ret;
 
+	mutex_lock(&pf->tc_mutex);
+
 	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
 	if (ret) {
 		dev_err(dev, "Query Port ETS failed\n");
 		goto dcb_error;
 	}
 
-	mutex_lock(&pf->tc_mutex);
-
 	if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
 		ice_cfg_etsrec_defaults(pf->hw.port_info);

On 2/23/26 23:00, Bart Van Assche wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> Move the mutex_lock() call up to prevent that DCB settings change after
> the ice_query_port_ets() call.
> 
> This patch fixes a bug in an error path. Without this patch pf->tc_mutex
> may be unlocked in an error path without having been locked. This bug has
> been detected by the clang thread-safety analyzer.

Thank you. This is a good catch, and current error path is obviously
wrong, in the way you have described.

 From the commit msg alone, it is not clear why moving the lock up,
instead moving the unlock to match the lock order, is right.
But from the code, I see that you are right - we call
ice_query_port_ets() again under the lock some 20 lines below (L566)

Let us know If you want to enhance your commit message (including the
subject), as it looks like a big quasi-automated series.

> 
> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index bd77f1c001ee..78ded6876581 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -537,14 +537,14 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>   	struct ice_dcbx_cfg *err_cfg;
>   	int ret;
>   
> +	mutex_lock(&pf->tc_mutex);
> +
>   	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
>   	if (ret) {
>   		dev_err(dev, "Query Port ETS failed\n");
>   		goto dcb_error;
>   	}
>   
> -	mutex_lock(&pf->tc_mutex);
> -
>   	if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
>   		ice_cfg_etsrec_defaults(pf->hw.port_info);
>

On 2/24/26 12:40 AM, Przemek Kitszel wrote:
> On 2/23/26 23:00, Bart Van Assche wrote:
>> From: Bart Van Assche <bvanassche@acm.org>
>>
>> Move the mutex_lock() call up to prevent that DCB settings change after
>> the ice_query_port_ets() call.
>>
>> This patch fixes a bug in an error path. Without this patch pf->tc_mutex
>> may be unlocked in an error path without having been locked. This bug has
>> been detected by the clang thread-safety analyzer.
> 
> Thank you. This is a good catch, and current error path is obviously
> wrong, in the way you have described.
> 
> From the commit msg alone, it is not clear why moving the lock up,
> instead moving the unlock to match the lock order, is right.

Please take a look at the prior discussion of this change, available 
here: 
https://lore.kernel.org/all/61e3cc7a-af79-48e4-acb6-8ac7c8d2552c@intel.com/

> But from the code, I see that you are right - we call
> ice_query_port_ets() again under the lock some 20 lines below (L566)
> 
> Let us know If you want to enhance your commit message (including the
> subject), as it looks like a big quasi-automated series.

I have been asked to split this patch series and to send it directly to
the maintainers of the modified subsystems. Suggestions for how to
improve the patch description are welcome.

Thanks,

Bart.

On 2/24/26 18:43, Bart Van Assche wrote:
> On 2/24/26 12:40 AM, Przemek Kitszel wrote:
>> On 2/23/26 23:00, Bart Van Assche wrote:
>>> From: Bart Van Assche <bvanassche@acm.org>
>>>
>>> Move the mutex_lock() call up to prevent that DCB settings change after
>>> the ice_query_port_ets() call.

s/./, as the other call in ice_dcb_rebuild() is/

>>>
>>> This patch fixes a bug in an error path. Without this patch pf->tc_mutex
>>> may be unlocked in an error path without having been locked. 

suggestion:
This also fixes a bug in an error path, as before taking the first
"goto dcb_error" in the function jumped over mutex_lock() to
mutex_unlock()

This bug
>>> has
>>> been detected by the clang thread-safety analyzer.
>>
>> Thank you. This is a good catch, and current error path is obviously
>> wrong, in the way you have described.
>>
>> From the commit msg alone, it is not clear why moving the lock up,
>> instead moving the unlock to match the lock order, is right.
> 
> Please take a look at the prior discussion of this change, available 
> here: https://lore.kernel.org/all/61e3cc7a-af79-48e4- 
> acb6-8ac7c8d2552c@intel.com/
> 
>> But from the code, I see that you are right - we call
>> ice_query_port_ets() again under the lock some 20 lines below (L566)
>>
>> Let us know If you want to enhance your commit message (including the
>> subject), as it looks like a big quasi-automated series.
> 
> I have been asked to split this patch series and to send it directly to
> the maintainers of the modified subsystems. Suggestions for how to
> improve the patch description are welcome.

ok, that aligns with what I wanted too,

see my suggestions inline above, and update Subject, perhaps:

Subject: ice: fix locking in ice_dcb_rebuild()

please send with the "iwl-net" prefix, as a separate single-patch series

> 
> Thanks,
> 
> Bart.
>