RE: [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags

[lizetao <lizetao1@huawei.com>]

Hi,

> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Wednesday, January 15, 2025 12:22 AM
> To: lizetao <lizetao1@huawei.com>; Jens Axboe <axboe@kernel.dk>
> Cc: io-uring@vger.kernel.org
> Subject: Re: [PATCH] io_uring/io-wq: Fix a small time window for reading work-
> >flags
> 
> On 1/14/25 02:06, lizetao wrote:
> > There is a small time window that is modified by other tasks after
> > reading work->flags. It is changed to read before use, which is more
> 
> Can you elaborate on what races with what? I don't immediately see any race
> here.

There is such a race context:
	
	worker												process
io_worker_handle_work:										IORING_OP_ASYNC_CANCEL
	io_wq_enqueue										__io_wq_worker_cancel
		work_flags = atomic_read(&work->flags);	// no IO_WQ_WORK_CANCEL		
													atomic_or(IO_WQ_WORK_CANCEL, &work->flags);
		if (work_flags & IO_WQ_WORK_CANCEL)	// false

There seems to be a small time window here, resulting in the latest flags not being used.

> 
> > in line with the semantics of atoms.
> > Fixes: 3474d1b93f89 ("io_uring/io-wq: make io_wq_work flags atomic")
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > ---
> >   io_uring/io-wq.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index
> > a38f36b68060..75096e77b1fe 100644
> > --- a/io_uring/io-wq.c
> > +++ b/io_uring/io-wq.c
> > @@ -932,7 +932,6 @@ static bool io_wq_work_match_item(struct
> io_wq_work *work, void *data)
> >   void io_wq_enqueue(struct io_wq *wq, struct io_wq_work *work)
> >   {
> >   	struct io_wq_acct *acct = io_work_get_acct(wq, work);
> > -	unsigned int work_flags = atomic_read(&work->flags);
> >   	struct io_cb_cancel_data match = {
> >   		.fn		= io_wq_work_match_item,
> >   		.data		= work,
> > @@ -945,7 +944,7 @@ void io_wq_enqueue(struct io_wq *wq, struct
> io_wq_work *work)
> >   	 * been marked as one that should not get executed, cancel it here.
> >   	 */
> >   	if (test_bit(IO_WQ_BIT_EXIT, &wq->state) ||
> > -	    (work_flags & IO_WQ_WORK_CANCEL)) {
> > +	    (atomic_read(&work->flags) & IO_WQ_WORK_CANCEL)) {
> >   		io_run_cancel(work, wq);
> >   		return;
> >   	}
> > @@ -959,7 +958,7 @@ void io_wq_enqueue(struct io_wq *wq, struct
> io_wq_work *work)
> >   	do_create = !io_wq_activate_free_worker(wq, acct);
> >   	rcu_read_unlock();
> >
> > -	if (do_create && ((work_flags & IO_WQ_WORK_CONCURRENT) ||
> > +	if (do_create && ((atomic_read(&work->flags) &
> > +IO_WQ_WORK_CONCURRENT) ||
> >   	    !atomic_read(&acct->nr_running))) {
> >   		bool did_create;
> >
> 
> --
> Pavel Begunkov


---
Li Zetao