* [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags
@ 2025-01-14 2:06 lizetao
2025-01-14 16:22 ` Pavel Begunkov
0 siblings, 1 reply; 4+ messages in thread
From: lizetao @ 2025-01-14 2:06 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov; +Cc: [email protected]
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 <[email protected]>
---
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;
--
2.33.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags
2025-01-14 2:06 [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags lizetao
@ 2025-01-14 16:22 ` Pavel Begunkov
2025-01-15 1:50 ` lizetao
0 siblings, 1 reply; 4+ messages in thread
From: Pavel Begunkov @ 2025-01-14 16:22 UTC (permalink / raw)
To: lizetao, Jens Axboe; +Cc: [email protected]
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 <[email protected]>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags
2025-01-14 16:22 ` Pavel Begunkov
@ 2025-01-15 1:50 ` lizetao
2025-01-15 12:55 ` Pavel Begunkov
0 siblings, 1 reply; 4+ messages in thread
From: lizetao @ 2025-01-15 1:50 UTC (permalink / raw)
To: Pavel Begunkov, Jens Axboe; +Cc: [email protected]
Hi,
> -----Original Message-----
> From: Pavel Begunkov <[email protected]>
> Sent: Wednesday, January 15, 2025 12:22 AM
> To: lizetao <[email protected]>; Jens Axboe <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> > ---
> > 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags
2025-01-15 1:50 ` lizetao
@ 2025-01-15 12:55 ` Pavel Begunkov
0 siblings, 0 replies; 4+ messages in thread
From: Pavel Begunkov @ 2025-01-15 12:55 UTC (permalink / raw)
To: lizetao, Jens Axboe; +Cc: [email protected]
On 1/15/25 01:50, lizetao wrote:
> Hi,
>
>> -----Original Message-----
>> From: Pavel Begunkov <[email protected]>
>> Sent: Wednesday, January 15, 2025 12:22 AM
>> To: lizetao <[email protected]>; Jens Axboe <[email protected]>
>> Cc: [email protected]
>> 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.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-15 12:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-14 2:06 [PATCH] io_uring/io-wq: Fix a small time window for reading work->flags lizetao
2025-01-14 16:22 ` Pavel Begunkov
2025-01-15 1:50 ` lizetao
2025-01-15 12:55 ` Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox