[PATCH iwl-next] ice: call mutex_lock() before mutex_unlock() in ice_dcb_rebuild()

[PATCH iwl-next] ice: call mutex_lock() before mutex_unlock() in ice_dcb_rebuild()

[Aleksandr Loktionov]

From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>

In ice_dcb_rebuild(), the first call to ice_query_port_ets() is checked
for error, and on failure jumps to the 'dcb_error' label. The error path
always calls mutex_unlock(&pf->tc_mutex), but at this point in the code
the mutex has never been locked -- mutex_lock() came after the check.

Releasing a mutex that was not locked is undefined behaviour and can
cause a deadlock or crash. Fix this by moving mutex_lock() to before
the error check, so that 'dcb_error' always pairs with a prior lock.

Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
Signed-off-by: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 3 +--
 1 file changed, 1 insertion(+), 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 bd77f1c..d516734 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -538,12 +538,11 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	int ret;
 
 	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
+	mutex_lock(&pf->tc_mutex);
 	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);
 
-- 
2.52.0

Re: [Intel-wired-lan] [PATCH iwl-next] ice: call mutex_lock() before mutex_unlock() in ice_dcb_rebuild()

[Przemek Kitszel]

On 3/20/26 06:05, Aleksandr Loktionov wrote:
> From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> 
> In ice_dcb_rebuild(), the first call to ice_query_port_ets() is checked
> for error, and on failure jumps to the 'dcb_error' label. The error path
> always calls mutex_unlock(&pf->tc_mutex), but at this point in the code
> the mutex has never been locked -- mutex_lock() came after the check.
> 
> Releasing a mutex that was not locked is undefined behaviour and can
> cause a deadlock or crash. Fix this by moving mutex_lock() to before
> the error check, so that 'dcb_error' always pairs with a prior lock.
> 
> Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
> Signed-off-by: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

NACK

there is a better fix proposed by community, we are awaiting v2
https://patchew.org/linux/20260223220102.2158611-1-bart.vanassche@linux.dev/20260223220102.2158611-20-bart.vanassche@linux.dev/

> ---
>   drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 3 +--
>   1 file changed, 1 insertion(+), 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 bd77f1c..d516734 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -538,12 +538,11 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>   	int ret;
>   
>   	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);

the difference is to move mutex_lock() over ice_query_port_ets()

> +	mutex_lock(&pf->tc_mutex);
>   	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);
>   

RE: [Intel-wired-lan] [PATCH iwl-next] ice: call mutex_lock() before mutex_unlock() in ice_dcb_rebuild()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Sent: Friday, March 20, 2026 11:13 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Bart Van
> Assche <bvanassche@acm.org>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
> Wieczerzycka, Katarzyna <katarzyna.wieczerzycka@intel.com>; Nguyen,
> Anthony L <anthony.l.nguyen@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ice: call mutex_lock()
> before mutex_unlock() in ice_dcb_rebuild()
> 
> On 3/20/26 06:05, Aleksandr Loktionov wrote:
> > From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> >
> > In ice_dcb_rebuild(), the first call to ice_query_port_ets() is
> > checked for error, and on failure jumps to the 'dcb_error' label.
> The
> > error path always calls mutex_unlock(&pf->tc_mutex), but at this
> point
> > in the code the mutex has never been locked -- mutex_lock() came
> after the check.
> >
> > Releasing a mutex that was not locked is undefined behaviour and can
> > cause a deadlock or crash. Fix this by moving mutex_lock() to before
> > the error check, so that 'dcb_error' always pairs with a prior lock.
> >
> > Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
> > Signed-off-by: Katarzyna Wieczerzycka
> > <katarzyna.wieczerzycka@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> NACK
> 
> there is a better fix proposed by community, we are awaiting v2
> https://patchew.org/linux/20260223220102.2158611-1-
> bart.vanassche@linux.dev/20260223220102.2158611-20-
> bart.vanassche@linux.dev/
> 
> > ---
> >   drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 3 +--
> >   1 file changed, 1 insertion(+), 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 bd77f1c..d516734 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> > @@ -538,12 +538,11 @@ void ice_dcb_rebuild(struct ice_pf *pf)
> >   	int ret;
> >
> >   	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf),
> > NULL);
> 
> the difference is to move mutex_lock() over ice_query_port_ets()
> 
> > +	mutex_lock(&pf->tc_mutex);
> >   	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);
> >

Withdrawing this patch (just in case NACK is not enough).

Bart Van Assche's series covers the same fix with a more complete
approach (moving mutex_lock() above ice_query_port_ets()). His v4
has been reviewed and accepted.

With the best regards
Alex