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

[Jens Axboe <axboe@kernel.dk>]

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.

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?

-- 
Jens Axboe