diff mbox series

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

Message ID 5fd306d40ebb4da0a657da9a9be5cec1@huawei.com (mailing list archive)
State New
Headers show
Series io_uring/io-wq: Fix a small time window for reading work->flags | expand

Commit Message

lizetao Jan. 14, 2025, 2:06 a.m. UTC 
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
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(-)

Comments

Pavel Begunkov Jan. 14, 2025, 4:22 p.m. UTC | #1 
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.

> 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;
>
lizetao Jan. 15, 2025, 1:50 a.m. UTC | #2 
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
Pavel Begunkov Jan. 15, 2025, 12:55 p.m. UTC | #3 
On 1/15/25 01:50, lizetao wrote:
> 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);

													^^^

That can't happen, the request is not discoverable via iowq yet.

> 		if (work_flags & IO_WQ_WORK_CANCEL)	// false

This check is for requests that came with the flag already set.
diff mbox series

Patch

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;