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

[Almog Khaikin <almogkh@gmail.com>]

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.