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

[Guoqing Jiang]

Hi,

On 11/8/24 10:49, Chengen Du wrote:
> From: Jens Axboe <axboe at kernel.dk>
>
> CVE-2023-21400
>
> BugLink: https://bugs.launchpad.net/bugs/2078659
>
> No direct upstream commit exists for this issue. It was fixed in
> 5.18 as part of a larger rework of the completion side.
>
> io_commit_cqring() writes the CQ ring tail to make it visible, but it
> also kicks off any deferred work we have. A ring setup with IOPOLL
> does not need any locking around the CQ ring updates, as we're always
> under the ctx uring_lock. But if we have deferred work that needs
> processing, then io_queue_deferred() assumes that the completion_lock
> is held, as it is for !IOPOLL.
>
> Add a lockdep assertion to check and document this fact, and have
> io_iopoll_complete() check if we have deferred work and run that
> separately with the appropriate lock grabbed.

IIUC, the change in this patch doesn't align with the above description.

> Cc: stable at vger.kernel.org # 5.10, 5.15
> Reported-by: dghost david <daviduniverse18 at gmail.com>
> Signed-off-by: Jens Axboe <axboe at kernel.dk>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (backported from commit 810e401b34c4c4c244d8b93b9947ea5b3d4d49f8 linux-5.10.y)
> [chengen - apply the locking logic to the corresponding functions /
> use spin_lock_irq because the same lock is used in the timer callback
> function.]
> Signed-off-by: Chengen Du <chengen.du at canonical.com>
> ---
>   fs/io_uring.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 875dd8e0f766..aa75c32f2d16 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -512,6 +512,8 @@ static void __io_commit_cqring(struct io_ring_ctx *ctx)
>   {
>   	struct io_rings *rings = ctx->rings;
>   
> +	lockdep_assert_held(&ctx->completion_lock);
> +
>   	if (ctx->cached_cq_tail != READ_ONCE(rings->cq.tail)) {
>   		/* order cqe stores with ring update */
>   		smp_store_release(&rings->cq.tail, ctx->cached_cq_tail);
> @@ -841,7 +843,9 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
>   		}
>   	}
>   
> +	spin_lock_irq(&ctx->completion_lock);
>   	io_commit_cqring(ctx);
> +	spin_unlock_irq(&ctx->completion_lock);
>   	io_free_req_many(ctx, reqs, &to_free);
>   }

For focal, io_commit_cqring was implemented between L577 [1] and L595 
[2]. I think the part
from L581 to L582 have been changed to io_flush_timeouts() per commit  
360428f8c0cd
("io_uring: move timeouts flushing to a helper").  And 
io_queue_deferred() was based on the
code between L586 and L594 per these commits:

045189452210 io_uring: separate DRAIN flushing into a cold path
87987898a1db io_uring: remove REQ_F_IO_DRAINED
1b4a51b6d03d io_uring: drain next sqe instead of shadowing

Personally, I'd more prefer similar change as in SUSE's fix instead of 
lock the whole io_commit_cqring function.
[1]. 
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/tree/fs/io_uring.c?h=master-next#n577
[2]. 
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/tree/fs/io_uring.c?h=master-next#n595

Thanks,
Guoqing