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

[sashiko-bot@kernel.org]

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