Re: [PATCH 19/62] ice: Fix a locking bug in an error path

[Przemek Kitszel]

On 2/23/26 23:00, Bart Van Assche wrote:
> From: Bart Van Assche <bvanassche@xxxxxxx>
>
> 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@xxxxxxxxx>
> Cc: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx
> Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
>   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);
>