[PATCH v2 rdma-rc] RDMA/ucma: free a queued CONNECT_REQUEST with its new context

[PATCH v2 rdma-rc] RDMA/ucma: free a queued CONNECT_REQUEST with its new context

[Yuehong Wu]

ucma_cleanup_ctx_events() only frees the events that belong to the
context being destroyed.  A CONNECT_REQUEST uevent that has not been
delivered to user space is queued on the listening context, but it also
stores a plain pointer to the newly created connection context in
uevent->conn_req_ctx.  That context is already live in ctx_table because
ucma_finish_ctx() runs before the event is queued, so it can be
destroyed without ever fetching the event: the ids are allocated from a
global xarray starting at the lowest free value, and DESTROY_ID only
requires the context to belong to the same ucma file.

If user space destroys that context while the event is still queued, i.e.
before it fetches it with get_event, the event is skipped here and stays
in the listener's event list with a pointer to the freed context.
Destroying the listener later
(which is what closing its file does) reaches the
uevent->conn_req_ctx->id lookup in the xa_cmpxchg() below with a
dangling pointer, and when the freed context was reallocated the
comparison can match an unrelated live context, which is then detached
from the id table and freed.

The match in commit 8ae291cc95e4 ("RDMA/ucma: Do not miss ctx
destruction steps in some cases") replaced "uevent->ctx == ctx ||
uevent->conn_req_ctx == ctx" with a plain "uevent->ctx != ctx" test and
lost the case where the new context is the one being destroyed.

Free the event in that case and return the backlog slot it consumed.

Fixes: 8ae291cc95e4 ("RDMA/ucma: Do not miss ctx destruction steps in some cases")
Cc: <stable@vger.kernel.org>
Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
---
v2: no code change.  Reword the comment that was unclear and correct the
description of how the context is reached before get_event: its id is
predictable, not disclosed to user space.  Also mention both directions
of the ucma_migrate_id() cross-file case.

Validated in a QEMU/KVM guest running this tree with CONFIG_KASAN=y and
CONFIG_RDMA_RXE=y.  A reproducer uses an rxe device, queues a
CONNECT_REQUEST, sees it with poll() without fetching it, finds the new
connection id by trying the small integer ids that belong to its own
file, destroys that id and then the listener.  The unpatched kernel
reports

  BUG: KASAN: slab-use-after-free in ucma_destroy_private_ctx+0x1c0/0x4f0
  Read of size 4 at addr ff11000102295a00 by task c9repro/302
  Allocated by task 12:
   ucma_alloc_ctx+0x2b/0x170
   ucma_event_handler+0x1ad/0x3a0
  Freed by task 302:
   ucma_destroy_private_ctx+0x468/0x4f0
   ucma_destroy_id+0x180/0x1e0

With this patch the same sequence run four times produces no report, and
a probe shows the queued event is dropped when the new context is
destroyed.

Note: ucma_migrate_id() only moves queued events whose uevent->ctx is
the migrated context.  Moving the new context away leaves the event on
the listener's file, while moving the listener moves the event away but
leaves the connection context behind; both cross-file cases predate
8ae291cc95e4 and are left for a separate fix.

 drivers/infiniband/core/ucma.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -534,8 +534,25 @@ static int ucma_cleanup_ctx_events(struct ucma_context *ctx)
 	/* Cleanup events not yet reported to the user.*/
 	mutex_lock(&ctx->file->mut);
 	list_for_each_entry_safe(uevent, tmp, &ctx->file->event_list, list) {
-		if (uevent->ctx != ctx)
+		/*
+		 * A CONNECT_REQUEST that has not been delivered to user
+		 * space is queued on the listening context but stores a
+		 * pointer to the new connection context.  The new context is
+		 * live in ctx_table before the event is queued, so it can be
+		 * destroyed before the event is fetched; the event can then
+		 * no longer be acted upon.  Drop it instead of leaving a
+		 * dangling conn_req_ctx in the event list.
+		 */
+		if (uevent->ctx != ctx) {
+			if (uevent->resp.event == RDMA_CM_EVENT_CONNECT_REQUEST &&
+			    uevent->conn_req_ctx == ctx) {
+				list_del(&uevent->list);
+				/* Return the slot taken for this request. */
+				atomic_inc(&uevent->ctx->backlog);
+				kfree(uevent);
+			}
 			continue;
+		}

 		if (uevent->resp.event == RDMA_CM_EVENT_CONNECT_REQUEST &&
 		    xa_cmpxchg(&ctx_table, uevent->conn_req_ctx->id,

Re: [PATCH v2 rdma-rc] RDMA/ucma: free a queued CONNECT_REQUEST with its new context

[sashiko-bot]

> ucma_cleanup_ctx_events() only frees the events that belong to the
> context being destroyed.  A CONNECT_REQUEST uevent that has not been
> delivered to user space is queued on the listening context, but it also
> stores a plain pointer to the newly created connection context in
> uevent->conn_req_ctx.  That context is already live in ctx_table because
> ucma_finish_ctx() runs before the event is queued, so it can be
> destroyed without ever fetching the event: the ids are allocated from a
> global xarray starting at the lowest free value, and DESTROY_ID only
> requires the context to belong to the same ucma file.
> 
> If user space destroys that context while the event is still queued, i.e.
> before it fetches it with get_event, the event is skipped here and stays
> in the listener's event list with a pointer to the freed context.
> Destroying the listener later
> (which is what closing its file does) reaches the
> uevent->conn_req_ctx->id lookup in the xa_cmpxchg() below with a
> dangling pointer, and when the freed context was reallocated the
> comparison can match an unrelated live context, which is then detached
> from the id table and freed.
> 
> The match in commit 8ae291cc95e4 ("RDMA/ucma: Do not miss ctx
> destruction steps in some cases") replaced "uevent->ctx == ctx ||
> uevent->conn_req_ctx == ctx" with a plain "uevent->ctx != ctx" test and
> lost the case where the new context is the one being destroyed.
> 
> Free the event in that case and return the backlog slot it consumed.
> 
> Fixes: 8ae291cc95e4 ("RDMA/ucma: Do not miss ctx destruction steps in some cases")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-ucma-conn-req-ctx-v2-1-yuehongwu@cmss.chinamobile.com?part=1