Messages in this thread | | | From | Lai Jiangshan <> | Date | Thu, 2 Mar 2023 11:08:16 +0800 | Subject | Re: [PATCH] workqueue: avoid usage of list iterator after loop in __flush_workqueue() |
| |
On Thu, Mar 2, 2023 at 7:23 AM Jakob Koschel <jkl820.git@gmail.com> 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
| |