[PATCH] io_uring: fix memory ordering when SQPOLL thread goes to sleep

[PATCH] io_uring: fix memory ordering when SQPOLL thread goes to sleep

[Almog Khaikin]

Without a full memory barrier between the store to the flags and the
load of the SQ tail the two operations can be reordered and this can
lead to a situation where the SQPOLL thread goes to sleep while the
application writes to the SQ tail and doesn't see the wakeup flag.
This memory barrier pairs with a full memory barrier in the application
between its store to the SQ tail and its load of the flags.

Signed-off-by: Almog Khaikin <almogkh@gmail.com>
---
 fs/io_uring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 4715980e9015..221961be9213 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7613,6 +7613,13 @@ static int io_sq_thread(void *data)
 					needs_sched = false;
 					break;
 				}
+
+				/*
+				 * Ensure the store of the wakeup flag is not
+				 * reordered with the load of the SQ tail
+				 */
+				smp_mb();
+				
 				if (io_sqring_entries(ctx)) {
 					needs_sched = false;
 					break;
-- 
2.35.1

Re: [PATCH] io_uring: fix memory ordering when SQPOLL thread goes to sleep

[Almog Khaikin]

On 3/6/22 12:35, Almog Khaikin wrote:
> Without a full memory barrier between the store to the flags and the
> load of the SQ tail the two operations can be reordered and this can
> lead to a situation where the SQPOLL thread goes to sleep while the
> application writes to the SQ tail and doesn't see the wakeup flag.
> This memory barrier pairs with a full memory barrier in the application
> between its store to the SQ tail and its load of the flags.

The IOPOLL list is internal to the kernel, userspace doesn't interact
with it. AFAICT it can't cause any races with userspace so the check if
the list is empty seems unnecessary. The flags and the SQ tail are the
only things that are shared that can cause any problems when the kernel
thread goes to sleep so I think it's safe to remove that check.

The race here can result in a situation where the kernel thread goes to
sleep while the application updates the SQ tail and doesn't see the
NEED_WAKEUP flag. Checking the SQ tail after setting the wakeup flag
along with the full barrier would ensure that either we see the tail
update or the application sees the wakeup flag. The IOPOLL list doesn't
tie into any of this.

 fs/io_uring.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 4715980e9015..99af6607b770 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7608,11 +7608,12 @@ static int io_sq_thread(void *data)
 			list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
 				io_ring_set_wakeup_flag(ctx);
 
-				if ((ctx->flags & IORING_SETUP_IOPOLL) &&
-				    !wq_list_empty(&ctx->iopoll_list)) {
-					needs_sched = false;
-					break;
-				}
+				/*
+				 * Ensure the store of the wakeup flag is not
+				 * reordered with the load of the SQ tail
+				 */
+				smp_mb();
+
 				if (io_sqring_entries(ctx)) {
 					needs_sched = false;
 					break;
-- 
2.35.1

Re: [PATCH] io_uring: fix memory ordering when SQPOLL thread goes to sleep

[Jens Axboe]

On 3/6/22 6:32 AM, Almog Khaikin wrote:
> On 3/6/22 12:35, Almog Khaikin wrote:
>> Without a full memory barrier between the store to the flags and the
>> load of the SQ tail the two operations can be reordered and this can
>> lead to a situation where the SQPOLL thread goes to sleep while the
>> application writes to the SQ tail and doesn't see the wakeup flag.
>> This memory barrier pairs with a full memory barrier in the application
>> between its store to the SQ tail and its load of the flags.
> 
> The IOPOLL list is internal to the kernel, userspace doesn't interact
> with it. AFAICT it can't cause any races with userspace so the check if
> the list is empty seems unnecessary. The flags and the SQ tail are the
> only things that are shared that can cause any problems when the kernel
> thread goes to sleep so I think it's safe to remove that check.
> 
> The race here can result in a situation where the kernel thread goes to
> sleep while the application updates the SQ tail and doesn't see the
> NEED_WAKEUP flag. Checking the SQ tail after setting the wakeup flag
> along with the full barrier would ensure that either we see the tail
> update or the application sees the wakeup flag. The IOPOLL list doesn't
> tie into any of this.

I think you're mixing up two different things, and even if not, the
IOPOLL change should be a separate change.

The iopoll list check isn't about synchronizing with userspace, it's
about not going to sleep if we have entries to reap. If we're running
with IOPOLL|SQPOLL, then it's the sq poll thread that does the polling
and reaping.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix memory ordering when SQPOLL thread goes to sleep

[Almog Khaikin]

On 3/6/22 23:00, Jens Axboe wrote:
> On 3/6/22 6:32 AM, Almog Khaikin wrote:
>> On 3/6/22 12:35, Almog Khaikin wrote:
>>> Without a full memory barrier between the store to the flags and the
>>> load of the SQ tail the two operations can be reordered and this can
>>> lead to a situation where the SQPOLL thread goes to sleep while the
>>> application writes to the SQ tail and doesn't see the wakeup flag.
>>> This memory barrier pairs with a full memory barrier in the application
>>> between its store to the SQ tail and its load of the flags.
>>
>> The IOPOLL list is internal to the kernel, userspace doesn't interact
>> with it. AFAICT it can't cause any races with userspace so the check if
>> the list is empty seems unnecessary. The flags and the SQ tail are the
>> only things that are shared that can cause any problems when the kernel
>> thread goes to sleep so I think it's safe to remove that check.
>>
>> The race here can result in a situation where the kernel thread goes to
>> sleep while the application updates the SQ tail and doesn't see the
>> NEED_WAKEUP flag. Checking the SQ tail after setting the wakeup flag
>> along with the full barrier would ensure that either we see the tail
>> update or the application sees the wakeup flag. The IOPOLL list doesn't
>> tie into any of this.
> 
> I think you're mixing up two different things, and even if not, the
> IOPOLL change should be a separate change.
> 
> The iopoll list check isn't about synchronizing with userspace, it's
> about not going to sleep if we have entries to reap. If we're running
> with IOPOLL|SQPOLL, then it's the sq poll thread that does the polling
> and reaping.

I understand that, but the iopoll list check is already done earlier in
the function and if the list isn't empty, the timer is reset. Checking
again just a little later in the function and after writing the
NEED_WAKEUP flag seems unnecessary but regardless, I guess it's not
really relevant to this patch as it's not a bug that the check is
there. The patch in the original message along with the liburing pull
request should fix the race.