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

[Chengen Du]

On Fri, Nov 22, 2024 at 5:23 PM Guoqing Jiang
<guoqing.jiang at canonical.com> wrote:
>
>
>
> 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 🙂.
Thank you for bringing this to my attention—I truly appreciate it.

Upon revisiting the code in detail, I discovered a bug related to
calling kill_fasync under completion_lock.
While this is a separate existing issue in Focal, it would be
fantastic if we could address it together.

To resolve this, we need to backport at least the following two commits first:
0791015837f1 - io_uring: remove extra check in __io_commit_cqring
4aa84f2ffa81 - io_uring: don't kill fasync under completion_lock

Once these are backported, we can address the CVE in a manner similar
to the patch applied in linux-5.10.y.
I will prepare version 2 of the patch for this CVE.
>
> 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
This is precisely the issue we aim to resolve with the CVE.
>
> Thanks,
> Guoqing

Best regards,
Chengen Du