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

[PATCH 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 belongs to the listening context, but it also
keeps the newly created connection context in uevent->conn_req_ctx, and
that context is already reachable from user space because
ucma_finish_ctx() published its id before the event was queued.

If user space destroys the new id 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 a context that
ucma_destroy_private_ctx() then frees.  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: it
was never delivered to user space, so it can no longer be acted upon.

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>
---
Validated in a QEMU/KVM guest running this tree with CONFIG_KASAN=y and
CONFIG_RDMA_RXE=y.  A reproducer creates 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() can move the new context to another file, which
leaves its queued CONNECT_REQUEST in the original file's event list.
That cross-file case predates 8ae291cc95e4 and is left for a separate
fix.

 drivers/infiniband/core/ucma.c | 17 ++++++++++++++++-
 1 file changed, 16 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,23 @@ 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 belongs to the listening context but also holds the
+		 * new one.  If the new context is destroyed first, the id it
+		 * was published under is gone, so drop the event 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 rdma-rc] RDMA/ucma: free a queued CONNECT_REQUEST with its new context

[Leon Romanovsky]

On Mon, Nov 09, 2026 at 11:20:00AM +0800, Yuehong Wu wrote:
> 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 belongs to the listening context, but it also
> keeps the newly created connection context in uevent->conn_req_ctx, and
> that context is already reachable from user space because
> ucma_finish_ctx() published its id before the event was queued.
> 
> If user space destroys the new id 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 a context that
> ucma_destroy_private_ctx() then frees.  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: it
> was never delivered to user space, so it can no longer be acted upon.
> 
> 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>
> ---
> Validated in a QEMU/KVM guest running this tree with CONFIG_KASAN=y and
> CONFIG_RDMA_RXE=y.  A reproducer creates 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() can move the new context to another file, which
> leaves its queued CONNECT_REQUEST in the original file's event list.
> That cross-file case predates 8ae291cc95e4 and is left for a separate
> fix.
> 
>  drivers/infiniband/core/ucma.c | 17 ++++++++++++++++-
>  1 file changed, 16 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,23 @@ 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 belongs to the listening context but also holds the
> +		 * new one. 

I don't understand this sentence. conn_req_ctx is created and added to
event_list at the same function without any conditions.

Please explain with function call chain how your claim is possible.

Thanks

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

[Yuehong Wu]

Leon,

sorry for the unclear comment. The context is created and queued in the
same function, unconditionally, that part is not in dispute. What the
comment was trying to say is that the uevent and the new context have
independent lifetimes: the event is queued on the listener's file and
stays there until user space asks for it, while the new context is live
in ctx_table from one statement earlier and can be destroyed in between.

Here is the chain for the queueing side, ucma_connect_event_handler()
(inlined into ucma_event_handler()):

  cma_ib_req_handler()
    cma_cm_event_handler()
      ucma_event_handler()
        ucma_connect_event_handler()
          atomic_add_unless(&listen_ctx->backlog, -1, 0)
          ctx = ucma_alloc_ctx(listen_ctx->file)
                  xa_alloc(&ctx_table, &ctx->id, NULL, ...)   id reserved, value NULL
          uevent = ucma_create_uevent(listen_ctx, event)
                  uevent->ctx = listen_ctx
          uevent->conn_req_ctx = ctx
          uevent->resp.id = ctx->id
          mutex_lock(&ctx->file->mut)
          ucma_finish_ctx(ctx)
                  xa_store(&ctx_table, ctx->id, ctx, ...)     id becomes live
          list_add_tail(&uevent->list, &ctx->file->event_list)
          mutex_unlock(&ctx->file->mut)
          wake_up_interruptible(&ctx->file->poll_wait)

So two separate things happen here. The new context is live in
ctx_table before the event is queued, and delivery to user space is a
separate step that only happens when user space calls get_event; poll()
just reports a non-empty queue and returns no id. The event does not take
a reference on the new context, so nothing ties the context's lifetime to
the event's.

Note that both statements are under file->mut, but that only serialises
the event list, not the id table: DESTROY_ID takes the id out of
ctx_table under xa_lock and only then needs file->mut, in
ucma_cleanup_ctx_events(). So it either blocks until the event has been
queued or finds it already queued; either way the event is on the list
when the cleanup runs, and gets skipped.

The destroy side is ucma_destroy_id():

  ucma_destroy_id()
    _ucma_find_context(cmd.id, file)            xa_load, ctx->file == file
    __xa_cmpxchg(&ctx_table, ctx->id, ctx, XA_ZERO_ENTRY)
    ucma_destroy_private_ctx(ctx)
      ucma_close_id() -> rdma_destroy_id(ctx->cm_id)
      ucma_cleanup_ctx_events(ctx)
        mutex_lock(&ctx->file->mut)             the same file as the listener
        uevent->ctx != ctx -> continue          the event is skipped here
      kfree(ctx)

And later, when the listener goes away (DESTROY_ID on the listening id,
or close() of its file, which walks ctx_list):

  ucma_cleanup_ctx_events(listen_ctx)
    uevent->ctx == listen_ctx, event == RDMA_CM_EVENT_CONNECT_REQUEST
    xa_cmpxchg(&ctx_table, uevent->conn_req_ctx->id, ...)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^ read of freed memory

That is the slab-use-after-free. The id is not handed to user space
before get_event, but it is predictable: xa_alloc() hands out the lowest
free id, the table is global, and DESTROY_ID only requires the id to
belong to the same ucma file, so a process can destroy the context while
the event is still queued. My reproducer does exactly that: it uses an
rxe device, queues a CONNECT_REQUEST, sees the queue is readable without
fetching it, tries the small ids of its own file, destroys the new one
and then the listener.

The window is not a race, it is a stable state, and get_event cannot race
with it either: ucma_get_event() copies the response and does list_del()
under file->mut before dropping it, so once the event has been fetched
the destroy path can no longer see it. The reason it has to be handled at
all is that the id is gone; the event can then never be acted upon, and
leaving it queued makes the listener's cleanup dereference freed memory.

Commit 8ae291cc95e4 replaced

        if (uevent->ctx == ctx || uevent->conn_req_ctx == ctx)

with a plain "uevent->ctx != ctx", which lost the case where the context
being destroyed is the new one; before that commit the event was moved
out and freed. The patch restores that behaviour without restoring the condition itself
(that would destroy the context a second time): a new branch under the
"uevent->ctx != ctx" test drops the event, so the xa_cmpxchg() path added
by 8ae291cc95e4 is unchanged.  It also returns the backlog slot consumed
in ucma_connect_event_handler(), which the old code leaked.

One note on scope: this only covers the same-file case. ucma_migrate_id()
moves queued events only when uevent->ctx == ctx, so moving the new
context away leaves the event on the listener's file, and moving the
listener takes the event along but leaves the connection context behind.
Both cross-file cases predate 8ae291cc95e4 and are left for a separate
fix.

(For the KASAN report in the changelog: ucma_connect_event_handler() is
inlined into ucma_event_handler() and ucma_cleanup_ctx_events() into
ucma_destroy_private_ctx(), which is why the stacks show those names.)

I will follow up with a v2 that rewrites that comment; the code is
unchanged.

Thanks,
Yuehong