linux-kernel - Re: [PATCH] workqueue: avoid usage of list iterator after loop in __flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 2 Mar 2023 11:08:16 +0800
From:   Lai Jiangshan <jiangshanlai@...il.com>
To:     Jakob Koschel <jkl820.git@...il.com>
Cc:     Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org,
        Pietro Borrello <borrello@...g.uniroma1.it>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
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@...il.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