[PATCH v2] RDMA/cma: wait for addr_handler() to finish in rdma_destroy_id()

[PATCH v2] RDMA/cma: wait for addr_handler() to finish in rdma_destroy_id()

[Palla Raghunath]

syzbot hit a use-after-free of id_priv in addr_handler(). The bad read
is in debug_mutex_unlock(), from the last mutex_unlock() of
handler_mutex, and the memory was freed by ucma_close() ->
rdma_destroy_id().

addr_handler() moves the state from RDMA_CM_ADDR_QUERY to
RDMA_CM_ADDR_RESOLVED (or RDMA_CM_ADDR_BOUND on error) under
handler_mutex. If rdma_destroy_id() runs at that point, it waits on
handler_mutex. When addr_handler() unlocks, the destroying task can take
the mutex before mutex_unlock() has returned. It then sees a state other
than RDMA_CM_ADDR_QUERY, so cma_cancel_operation() skips
rdma_addr_cancel(), and _destroy_id() frees id_priv. mutex_unlock() in
the work then touches the freed lock:

  ib_addr work                         close()
  addr_handler()
    mutex_lock(handler_mutex)
    ADDR_QUERY -> ADDR_RESOLVED
    ...                                rdma_destroy_id()
                                         mutex_lock(handler_mutex)
    mutex_unlock(handler_mutex)
      owner cleared                      gets the mutex
                                         state != ADDR_QUERY, no cancel
                                         kfree(id_priv)
      debug_mutex_unlock(lock)  <- use-after-free

Documentation/locking/mutex-design.rst says mutex_unlock() may still
touch the mutex after another task has acquired it, so handler_mutex
can't be what keeps id_priv alive here. The comment in
cma_cancel_operation() assumes it can. Before commit 722c7b2bfead
("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()"),
addr_handler() held a reference on id_priv until after mutex_unlock(),
which covered this.

So in rdma_destroy_id(), call rdma_addr_cancel() before taking
handler_mutex if a resolve was ever started on this id. The req stays on
req_list until the callback returns, so rdma_addr_cancel() finds it and
cancel_delayed_work_sync() waits until addr_handler() is really done.
This doesn't deadlock with the work itself. When addr_handler() destroys
the id because the event handler returned non-zero, it uses
destroy_id_handler_unlock(), not rdma_destroy_id(). Event handlers also
run with handler_mutex held, so they can't call rdma_destroy_id() on
their own id anyway.

After that cancel, clear used_resolve_ip and make cma_cancel_operation()
check it, so the request isn't canceled a second time on the way out.
Every id with an addr request outstanding has used_resolve_ip set, so
the device removal path still cancels as before. The loopback and AF_IB
resolves also use RDMA_CM_ADDR_QUERY, but they never queue an addr
request, so there was nothing for rdma_addr_cancel() to do for them.

There's no reproducer from syzbot, so I made the window bigger with a
debug-only mdelay(1000) after the mutex_unlock() in addr_handler(),
followed by a read of handler_mutex.magic. A small test program creates
an id, resolves an address on an rxe device and closes the fd while the
work sits in that delay. Without this patch KASAN reports the same
slab-use-after-free as syzbot (1048 bytes into a kmalloc-2k object,
freed by ucma_close()). With it the test runs clean, and close() just
waits for the work to finish.

Fixes: 722c7b2bfead ("RDMA/{cma, core}: Avoid callback on rdma_addr_cancel()")
Reported-by: syzbot+ae549381b4daac2895b1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ae549381b4daac2895b1
Signed-off-by: Palla Raghunath <raghunathpalla.0209@gmail.com>
---
v2:
- Don't call rdma_addr_cancel() twice on the same id: clear
  used_resolve_ip after the early cancel in rdma_destroy_id() and check it
  in cma_cancel_operation() (Leon)
- v1: https://lore.kernel.org/all/20261004112613.18171-1-raghunathpalla.0209@gmail.com/

 drivers/infiniband/core/cma.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 337a49d1acf7..72138f40dd78 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1968,12 +1968,14 @@ static void cma_cancel_operation(struct rdma_id_private *id_priv,
 		/*
 		 * We can avoid doing the rdma_addr_cancel() based on state,
 		 * only RDMA_CM_ADDR_QUERY has a work that could still execute.
-		 * Notice that the addr_handler work could still be exiting
-		 * outside this state, however due to the interaction with the
-		 * handler_mutex the work is guaranteed not to touch id_priv
-		 * during exit.
+		 * The addr_handler work can still be finishing its
+		 * mutex_unlock() after it has left this state.
+		 * rdma_destroy_id() waits for that before it takes
+		 * handler_mutex, and clears used_resolve_ip so the request
+		 * isn't canceled twice.
 		 */
-		rdma_addr_cancel(&id_priv->id.route.addr.dev_addr);
+		if (id_priv->used_resolve_ip)
+			rdma_addr_cancel(&id_priv->id.route.addr.dev_addr);
 		break;
 	case RDMA_CM_ROUTE_QUERY:
 		cma_cancel_route(id_priv);
@@ -2121,6 +2123,20 @@ void rdma_destroy_id(struct rdma_cm_id *id)
 	struct rdma_id_private *id_priv =
 		container_of(id, struct rdma_id_private, id);
 
+	/*
+	 * addr_handler() can still be in mutex_unlock(&handler_mutex) after
+	 * it has moved the state on from RDMA_CM_ADDR_QUERY, and
+	 * mutex_unlock() may touch the mutex even after we have taken it.
+	 * Wait for the work to finish before we free id_priv. The req stays
+	 * on req_list until the callback returns, so rdma_addr_cancel() will
+	 * find it. Nothing can issue a new request now, so clear
+	 * used_resolve_ip and let cma_cancel_operation() skip its cancel.
+	 */
+	if (id_priv->used_resolve_ip) {
+		rdma_addr_cancel(&id->route.addr.dev_addr);
+		id_priv->used_resolve_ip = 0;
+	}
+
 	mutex_lock(&id_priv->handler_mutex);
 	destroy_id_handler_unlock(id_priv);
 }
-- 
2.34.1

Re: [PATCH v2] RDMA/cma: wait for addr_handler() to finish in rdma_destroy_id()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cma: UAF in addr_handler via device removal path
- [High] rdma: ABA UAF in rdma_addr_cancel due to matching req->addr pointer
- [Low] rdma: data race on id_priv->used_resolve_ip during concurrent teardown
--

commit 90d3632ce0f65b9c550b8fd703273d10f1bd0efb
Author: Palla Raghunath <raghunathpalla.0209@gmail.com>

RDMA/cma: wait for addr_handler() to finish in rdma_destroy_id()

This commit attempts to fix a use-after-free of id_priv in addr_handler()
when called via rdma_destroy_id(). It calls rdma_addr_cancel() before taking
handler_mutex to ensure the handler is fully finished, and clears
used_resolve_ip to prevent double cancellation.

Link: https://lore.kernel.org/all/20261004112613.18171-1-raghunathpalla.0209@gmail.com/

> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 337a49d1acf72..72138f40dd78e 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1968,12 +1968,14 @@ static void cma_cancel_operation(struct rdma_id_private *id_priv,
>  		/*
>  		 * We can avoid doing the rdma_addr_cancel() based on state,
>  		 * only RDMA_CM_ADDR_QUERY has a work that could still execute.
> -		 * Notice that the addr_handler work could still be exiting
> -		 * outside this state, however due to the interaction with the
> -		 * handler_mutex the work is guaranteed not to touch id_priv
> -		 * during exit.
> +		 * The addr_handler work can still be finishing its
> +		 * mutex_unlock() after it has left this state.
> +		 * rdma_destroy_id() waits for that before it takes
> +		 * handler_mutex, and clears used_resolve_ip so the request
> +		 * isn't canceled twice.
>  		 */
> -		rdma_addr_cancel(&id_priv->id.route.addr.dev_addr);
> +		if (id_priv->used_resolve_ip)

[Severity: High]
Does this leave a use-after-free window open if destruction happens via the
device removal path?

If cma_process_remove() processes device removal concurrently with
addr_handler(), it queues cma_send_device_removal_put().

addr_handler() acquires handler_mutex, updates the state to
RDMA_CM_ADDR_RESOLVED, attaches to the device, and drops the mutex.

cma_send_device_removal_put() then acquires the mutex, reads the updated state,
triggers the DEVICE_REMOVAL event, and if the ULP (like SRP) returns non-zero,
it directly calls _destroy_id(id_priv, state), bypassing rdma_destroy_id():

drivers/infiniband/core/cma.c:cma_send_device_removal_put() {
    ...
    if (cma_cm_event_handler(id_priv, &event)) {
        trace_cm_id_destroy(id_priv);
        _destroy_id(id_priv, state);
        return;
    }
    ...
}

Because cma_cancel_operation() ties cancellation exclusively to the
RDMA_CM_ADDR_QUERY state, the _destroy_id() call will pass
state = RDMA_CM_ADDR_RESOLVED. This skips the rdma_addr_cancel() call,
failing to wait for addr_handler() to finish, and immediately freeing
id_priv while addr_handler() is still accessing it during mutex_unlock().

[Severity: Low]
Does this lockless read of used_resolve_ip cause a data race?

When the ULP handles RDMA_CM_EVENT_DEVICE_REMOVAL by queueing an asynchronous
teardown task that calls rdma_destroy_id(), this read can execute concurrently
with the ULP's teardown task. If cma_process_remove() calls
cma_cancel_operation() after releasing handler_mutex, neither path holds a
lock protecting this variable. Should this be annotated with READ_ONCE() to
prevent KCSAN warnings?

> +			rdma_addr_cancel(&id_priv->id.route.addr.dev_addr);
>  		break;
>  	case RDMA_CM_ROUTE_QUERY:
>  		cma_cancel_route(id_priv);

[ ... ]

> @@ -2121,6 +2123,20 @@ void rdma_destroy_id(struct rdma_cm_id *id)
>  	struct rdma_id_private *id_priv =
>  		container_of(id, struct rdma_id_private, id);
>  
> +	/*
> +	 * addr_handler() can still be in mutex_unlock(&handler_mutex) after
> +	 * it has moved the state on from RDMA_CM_ADDR_QUERY, and
> +	 * mutex_unlock() may touch the mutex even after we have taken it.
> +	 * Wait for the work to finish before we free id_priv. The req stays
> +	 * on req_list until the callback returns, so rdma_addr_cancel() will
> +	 * find it. Nothing can issue a new request now, so clear
> +	 * used_resolve_ip and let cma_cancel_operation() skip its cancel.
> +	 */
> +	if (id_priv->used_resolve_ip) {
> +		rdma_addr_cancel(&id->route.addr.dev_addr);

[Severity: High]
Does matching the request by pointer in rdma_addr_cancel() introduce an ABA
use-after-free issue?

In addr_handler(), if the user's event handler returns non-zero (for example,
rds_rdma_cm_event_handler_cmn() can return a non-zero error code during
RDMA_CM_EVENT_ADDR_RESOLVED), destroy_id_handler_unlock() synchronously frees
the id_priv memory:

drivers/infiniband/core/cma.c:addr_handler() {
    ...
    if (cma_cm_event_handler(id_priv, &event)) {
        destroy_id_handler_unlock(id_priv);
        return;
    }
    ...
}

At this point, the old req is still on req_list while process_one_req() is
executing and has not yet acquired the spin_lock_bh(&lock) to remove it:

drivers/infiniband/core/addr.c:process_one_req() {
    ...
    cancel_delayed_work(&req->work);
    if (!list_empty(&req->list)) {
        list_del_init(&req->list);
        kfree(req);
    }
    ...
}

A concurrent thread can allocate a new id_priv that reuses the exact same memory
address (slab reuse). The new thread calls rdma_resolve_addr(), queueing a new
req to req_list with the exact same addr pointer.

When the new thread then calls rdma_destroy_id(), it invokes
rdma_addr_cancel(&new_id_priv->id.route.addr.dev_addr):

drivers/infiniband/core/addr.c:rdma_addr_cancel() {
    ...
    list_for_each_entry_safe(req, temp_req, &req_list, list) {
        if (req->addr == addr) {
            list_del_init(&req->list);
            found = req;
            break;
        }
    }
    ...
}

Because rdma_addr_cancel() stops at the first match, it finds and cancels the
OLD req instead of the NEW req. The NEW req remains active on req_list. After
the new id_priv is destroyed and freed, the NEW req's delayed work will
eventually fire, passing the freed id_priv to its callback and causing a
use-after-free.

> +		id_priv->used_resolve_ip = 0;

[Severity: Low]
Does this lockless write to used_resolve_ip cause a data race?

As noted above, this can execute concurrently with lockless reads in
cma_cancel_operation() during device removal. Should this be annotated with
WRITE_ONCE()?

> +	}
> +
>  	mutex_lock(&id_priv->handler_mutex);
>  	destroy_id_handler_unlock(id_priv);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011174846.59186-1-raghunathpalla.0209@gmail.com?part=1