Re: [PATCH] IB/isert: set ISER_CONN_DOWN only after the connection has been released

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] IB/isert: UAF on rem_wait in isert_connect_release() after ISER_CONN_DOWN
- [High] IB/isert: deadlock on handler_mutex in isert_connect_release() on device removal
--

commit dc719d99e88528503ed27ec0d20e073beee4b8f9
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

IB/isert: set ISER_CONN_DOWN only after the connection has been released

This commit moves the store setting isert_conn->state to ISER_CONN_DOWN from
isert_release_work() into isert_connect_release() right before waking
isert_conn->rem_wait. This is intended to prevent the
RDMA_CM_EVENT_DEVICE_REMOVAL handler in isert_cma_handler() from freeing
isert_conn while isert_connect_release() is still tearing down connection
resources.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..cd0ca3cf131cb 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -506,10 +506,18 @@ isert_connect_release(struct isert_conn *isert_conn)
>  
>  	isert_device_put(device);
>  
> -	if (isert_conn->dev_removed)
> +	if (isert_conn->dev_removed) {
> +		/*
> +		 * Publish ISER_CONN_DOWN only now.  The
> +		 * RDMA_CM_EVENT_DEVICE_REMOVAL handler waits for it and then
> +		 * frees isert_conn, so it must not become visible before the
> +		 * teardown above has finished using the connection.
> +		 */
> +		isert_conn->state = ISER_CONN_DOWN;
>  		wake_up_interruptible(&isert_conn->rem_wait);

[Severity: Critical]
Can publishing isert_conn->state = ISER_CONN_DOWN before calling
wake_up_interruptible(&isert_conn->rem_wait) cause a use-after-free on
isert_conn->rem_wait when racing with kfree(isert_conn) in
isert_cma_handler()?

In drivers/infiniband/ulp/isert/ib_isert.c, isert_cma_handler() handles
RDMA_CM_EVENT_DEVICE_REMOVAL with:

	case RDMA_CM_EVENT_DEVICE_REMOVAL:
		isert_conn = cma_id->qp->qp_context;
		isert_conn->dev_removed = true;
		isert_disconnected_handler(cma_id, event->event);
		wait_event_interruptible(isert_conn->rem_wait,
					 isert_conn->state == ISER_CONN_DOWN);
		kfree(isert_conn);

Because rem_wait is embedded inside struct isert_conn and
wait_event_interruptible() checks isert_conn->state == ISER_CONN_DOWN before
acquiring rem_wait.lock (and again after prepare_to_wait_event() before
schedule()), as soon as the store to isert_conn->state becomes visible on
another CPU, wait_event_interruptible() can return immediately and call
kfree(isert_conn) before wake_up_interruptible(&isert_conn->rem_wait) even
begins. In that fast-path case, no barrier in wake_up_interruptible() has
executed to order prior accesses in isert_connect_release() before
kfree(isert_conn), and wake_up_interruptible() -> __wake_up_common_lock() ->
spin_lock_irqsave(&wq_head->lock, flags) will then dereference and write to
the freed isert_conn->rem_wait.

Even when the waiter is asleep in schedule(), __wake_up_common_lock() invokes
autoremove_wake_function(), which wakes the task and removes wq_entry from
rem_wait.head via list_del_init_careful(&wq_entry->entry) before releasing
rem_wait.lock. The woken waiter can observe isert_conn->state ==
ISER_CONN_DOWN, see list_empty_careful(&wq_entry->entry) in finish_wait()
without taking rem_wait.lock, and call kfree(isert_conn) while
__wake_up_common_lock() in isert_connect_release() is still traversing
rem_wait.head and unlocking isert_conn->rem_wait.lock.

Would replacing rem_wait with a struct completion (using complete() and
wait_for_completion_interruptible(), which serialize completion state and
waiter exit under the completion's wait.lock) avoid this race?

> -	else
> +	} else {
>  		kfree(isert_conn);
> +	}
>  }
>  
>  static void
> @@ -2537,10 +2545,6 @@ static void isert_release_work(struct work_struct *work)
>  
>  	isert_info("Starting release conn %p\n", isert_conn);
>  
> -	mutex_lock(&isert_conn->mutex);
> -	isert_conn->state = ISER_CONN_DOWN;
> -	mutex_unlock(&isert_conn->mutex);

[Severity: High]
Does moving the ISER_CONN_DOWN transition from isert_release_work() to the end
of isert_connect_release() introduce a deadlock on id_priv->handler_mutex when
a normal connection teardown races with RDMA device removal?

In drivers/infiniband/ulp/isert/ib_isert.c, isert_connect_release() runs after
isert_release_work() drops the last reference and checks dev_removed before
destroying the CM ID:

static void
isert_connect_release(struct isert_conn *isert_conn)
{
	...
	if (isert_conn->cm_id &&
	    !isert_conn->dev_removed)
		rdma_destroy_id(isert_conn->cm_id);

If a connection begins normal teardown prior to device removal (for example,
via RDMA_CM_EVENT_DISCONNECTED or iscsit_close_connection() ->
isert_wait_conn()), isert_conn->state is set to ISER_CONN_TERMINATING with
isert_conn->dev_removed == false, and isert_release_work() invokes
isert_connect_release().

If isert_connect_release() observes !isert_conn->dev_removed and enters
rdma_destroy_id(isert_conn->cm_id) just as cma_remove_one() ->
cma_process_remove() -> cma_send_device_removal_put() acquires
id_priv->handler_mutex first:

CPU0 (isert_release_wq)                 CPU1 (device removal)
isert_release_work()
  isert_put_conn()
    isert_connect_release()
      sees !isert_conn->dev_removed
      rdma_destroy_id(cm_id)
                                        cma_send_device_removal_put()
                                          mutex_lock(&id_priv->handler_mutex)
                                          cma_ndev_work / cma_cm_event_handler
                                            isert_cma_handler()
                                              isert_conn->dev_removed = true
                                              isert_disconnected_handler()
                                                (sees ISER_CONN_TERMINATING)
                                              wait_event_interruptible(
                                                isert_conn->rem_wait,
                                                state == ISER_CONN_DOWN)
        mutex_lock(&id_priv->handler_mutex)

Because ISER_CONN_DOWN is now set only after rdma_destroy_id() returns in
isert_connect_release(), CPU0 blocks waiting for id_priv->handler_mutex inside
rdma_destroy_id() while CPU1 holds id_priv->handler_mutex waiting for
isert_conn->state == ISER_CONN_DOWN. Furthermore, if the interruptible wait on
CPU1 is interrupted by a signal, isert_cma_handler() frees isert_conn and
returns 1 to destroy cm_id while rdma_destroy_id() on CPU0 proceeds to destroy
cm_id a second time and then accesses the freed isert_conn.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026000733.4178856-1-yhlee@isslab.korea.ac.kr?part=1