io_uring/rsrc: require cloned buffers to share accounting contexts

Commit Message

Jann Horn Jan. 14, 2025, 5:49 p.m. UTC

When IORING_REGISTER_CLONE_BUFFERS is used to clone buffers from uring
instance A to uring instance B, where A and B use different MMs for
accounting, the accounting can go wrong:
If uring instance A is closed before uring instance B, the pinned memory
counters for uring instance B will be decremented, even though the pinned
memory was originally accounted through uring instance A; so the MM of
uring instance B can end up with negative locked memory.

Cc: [e-mail]
Closes: https://lore.kernel.org/r/[e-mail]
Fixes: 7cc2a6eadcd7 ("io_uring: add IORING_REGISTER_COPY_BUFFERS method")
Signed-off-by: Jann Horn <[e-mail]>
---
To be clear, I think this is a very minor issue, feel free to take your
time landing it.

I put a stable marker on this, but I'm ambivalent about whether this
issue even warrants landing a fix in stable - feel free to remove the
Cc stable marker if you think it's unnecessary.
---
 io_uring/rsrc.c | 7 +++++++
 1 file changed, 7 insertions(+)


---
base-commit: c45323b7560ec87c37c729b703c86ee65f136d75
change-id: 20250114-uring-check-accounting-4356f8b91c37

Comments

Jens Axboe Jan. 14, 2025, 5:57 p.m. UTC | #1

On 1/14/25 10:49 AM, Jann Horn wrote:
> When IORING_REGISTER_CLONE_BUFFERS is used to clone buffers from uring
> instance A to uring instance B, where A and B use different MMs for
> accounting, the accounting can go wrong:
> If uring instance A is closed before uring instance B, the pinned memory
> counters for uring instance B will be decremented, even though the pinned
> memory was originally accounted through uring instance A; so the MM of
> uring instance B can end up with negative locked memory.
> 
> Cc: [e-mail]
> Closes: https://lore.kernel.org/r/[e-mail]
> Fixes: 7cc2a6eadcd7 ("io_uring: add IORING_REGISTER_COPY_BUFFERS method")
> Signed-off-by: Jann Horn <[e-mail]>
> ---
> To be clear, I think this is a very minor issue, feel free to take your
> time landing it.
> 
> I put a stable marker on this, but I'm ambivalent about whether this
> issue even warrants landing a fix in stable - feel free to remove the
> Cc stable marker if you think it's unnecessary.

I'll just queue it up for 6.14. Let's just get it towards stable, if
nothing else it provides consistent behavior across kernels. IMHO that's
enough reason to move it to stable.

Jens Axboe Jan. 14, 2025, 6:10 p.m. UTC | #2

On Tue, 14 Jan 2025 18:49:00 +0100, Jann Horn wrote:
> When IORING_REGISTER_CLONE_BUFFERS is used to clone buffers from uring
> instance A to uring instance B, where A and B use different MMs for
> accounting, the accounting can go wrong:
> If uring instance A is closed before uring instance B, the pinned memory
> counters for uring instance B will be decremented, even though the pinned
> memory was originally accounted through uring instance A; so the MM of
> uring instance B can end up with negative locked memory.
> 
> [...]

Applied, thanks!

[1/1] io_uring/rsrc: require cloned buffers to share accounting contexts
      commit: 19d340a2988d4f3e673cded9dde405d727d7e248

Best regards,

Patch

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 077f84684c18a0b3f5e622adb4978b6a00353b2f..caecc18dd5be03054ae46179bc0918887bf609a4 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -931,6 +931,13 @@  static int io_clone_buffers(struct io_ring_ctx *ctx, struct io_ring_ctx *src_ctx
 	int i, ret, off, nr;
 	unsigned int nbufs;
 
+	/*
+	 * Accounting state is shared between the two rings; that only works if
+	 * both rings are accounted towards the same counters.
+	 */
+	if (ctx->user != src_ctx->user || ctx->mm_account != src_ctx->mm_account)
+		return -EINVAL;
+
 	/* if offsets are given, must have nr specified too */
 	if (!arg->nr && (arg->dst_off || arg->src_off))
 		return -EINVAL;