MIME-Version: 1.0
References: <20230302-workqueue-avoid-iter-after-loop-v1-1-d83324743b99@gmail.com>
In-Reply-To: <20230302-workqueue-avoid-iter-after-loop-v1-1-d83324743b99@gmail.com>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Thu, 2 Mar 2023 11:08:16 +0800
Message-ID: <CAJhGHyCAGd3cK_hmV6sQZfODXwFT82g37xPhOcSWNO3ktH3V2g@mail.gmail.com>
Subject: Re: [PATCH] workqueue: avoid usage of list iterator after loop in __flush_workqueue()
To:     Jakob Koschel <jkl820.git@gmail.com>
Cc:     Tejun Heo <tj@kernel.org>, linux-kernel@vger.kernel.org,
        Pietro Borrello <borrello@diag.uniroma1.it>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 2, 2023 at 7:23=E2=80=AFAM 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 =3D list_first_entry(&wq->flusher_queue, struct wq_flusher, list);

Thanks
Lai