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

[Przemek Kitszel]

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@xxxxxxx>
> > >
> > > 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@xxxxxxxxx/
>
> > 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.