[SRU][F][PATCH 1/1] io_uring: ensure IOPOLL locks around deferred work

[Guoqing Jiang]

On 11/21/24 15:21, Chengen Du wrote:
> Hi Guoqing,
>
> Thank you for your response.
>
> Before submitting this patch, I analyzed the level of effort required
> to make our patch align more closely with SUSE’s implementation.
> I understand that narrowing the lock range would be beneficial, but
> there are several considerations to take into account.
>
> Regarding commit 810e401b34c4 (io_uring: ensure IOPOLL locks around
> deferred work), it uses the io_commit_needs_flush function to
> determine whether to invoke __io_commit_cqring_flush.
> In the Focal version, we lack the attributes used in the
> io_commit_needs_flush logic.
> Additionally, the __io_commit_cqring function in Focal includes tasks
> beyond simply updating rings->cq.tail.
> This raises uncertainty about the tangible benefits of separating the
> logic in our context.
>
> Moreover, the commits you've referenced cannot be backported unchanged.
> We would need to investigate them in detail to prevent regressions.
> The execution order within the io_commit_cqring function differs as
> well—__io_commit_cqring is invoked in the middle of the flow instead
> of at the head or tail.
> To narrow the lock range as SUSE has done, we would need to backport
> other commits unrelated to the CVE to adjust the flow accordingly.
>
> Another factor to consider is that the liburing package, which
> provides user-space helpers for kernel io_uring functionality, is not
> available in Focal.
> The CVE in question has also existed for some time in Focal.
> Therefore, we need to weigh the necessity of backporting multiple
> unrelated commits to address a CVE that may not significantly impact
> many users, especially given the risk of introducing regressions to
> io_uring functionality.

Don't get me wrong, I didn't ask to backport those commits which I 
listed in last mail,
what I preferred is to follow as much as commit 810e401b34 did which 
only lock flush
relevant function.

> These are my initial thoughts, though I recognize there may be aspects
> I have overlooked.
> If you believe it is worthwhile to proceed with separating the logic,
> I am happy to focus on that approach.

I personally think it is better to separate the logic, but it is your 
call 🙂.

And I am not sure add "lockdep_assert_held(&ctx->completion_lock)" in 
__io_commit_cqring
is appropriate, looks this path doesn't hold completion_lock if I am not 
misreading.

io_uring_release -> io_ring_ctx_wait_and_kill
                                         -> io_iopoll_reap_events
                                              -> io_iopoll_getevents
                                                   -> io_do_iopoll
                                                        -> 
io_iopoll_complete
                                                             -> 
io_commit_cqring  -> __io_commit_cqring

Thanks,
Guoqing