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

[Almog Khaikin <almogkh@gmail.com>]

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