Re: [PATCH] workqueue: avoid usage of list iterator after loop in __flush_workqueue()

[Lai Jiangshan]

On Thu, Mar 2, 2023 at 7:23 AM Jakob Koschel <jkl820.git@xxxxxxxxx> wrote:
>
> If the list_for_each_entry_safe() iteration never breaks, 'next' would
> contain an invalid pointer past the iterator loop. To ensure 'next' is
> always valid, we only set it if the correct element was found. That
> allows adding a WARN_ON_ONCE in case the code works incorrectly,
> exposing currently undetectable potential bugs.

Hello

In the code, if I did not miss anything important, I don't think there are any
way that the 'next' is invalid because it is used after this check:
        if (list_empty(&wq->flusher_queue))

which means the list_for_each_entry_safe() iteration did break and
the 'next' is valid.
(the code also moves entries from &wq->flusher_overflow to
wq->flusher_queue, but it only happens when wq->flusher_queue
is not empty because the number of colors > 2)

The logic is quite complicated and I agree with you that we
should avoid using the 'next' after the loop directly and remove
any possible misunderstanding/confusion.

But I don't want to make the code even more complicated by
adding more variables.

I prefer reinitializing the "next" before it is reused as:
   next = list_first_entry(&wq->flusher_queue, struct wq_flusher, list);

Thanks
Lai