Re: [PATCH] io_uring: fix io_kiocb.flags modification race in IOPOLL mode

[Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>]

hi,

> On 6/11/20 3:25 AM, Xiaoguang Wang wrote:
>> While testing io_uring in arm, we found sometimes io_sq_thread() keeps
>> polling io requests even though there are not inflight io requests in
>> block layer. After some investigations, found a possible race about
>> io_kiocb.flags, see below race codes:
>>    1) in the end of io_write() or io_read()
>>      req->flags &= ~REQ_F_NEED_CLEANUP;
>>      kfree(iovec);
>>      return ret;
>>
>>    2) in io_complete_rw_iopoll()
>>      if (res != -EAGAIN)
>>          req->flags |= REQ_F_IOPOLL_COMPLETED;
>>
>> In IOPOLL mode, io requests still maybe completed by interrupt, then
>> above codes are not safe, concurrent modifications to req->flags, which
>> is not protected by lock or is not atomic modifications. I also had
>> disassemble io_complete_rw_iopoll() in arm:
>>     req->flags |= REQ_F_IOPOLL_COMPLETED;
>>     0xffff000008387b18 <+76>:    ldr     w0, [x19,#104]
>>     0xffff000008387b1c <+80>:    orr     w0, w0, #0x1000
>>     0xffff000008387b20 <+84>:    str     w0, [x19,#104]
>>
>> Seems that the "req->flags |= REQ_F_IOPOLL_COMPLETED;" is  load and
>> modification, two instructions, which obviously is not atomic.
>>
>> To fix this issue, add a new iopoll_completed in io_kiocb to indicate
>> whether io request is completed.
> 
> Long term, I want to ensure that IOPOLL irq completions are illegal, it
> should not be enabled (or possible) if the driver doesn't do pure polled
> completions.
Yes, agree.

> 
> Short term, I think your fix is fine, but should be turned into using
> READ_ONCE/WRITE_ONCE for the reading/setting of ->iopoll_completed.
> Can you resend it with that?
OK, I'll prepare one now.

Regards,
Xiaoguang Wang

>