Re: [PATCH v2 RESEND] IB/isert: tear connections down from an ib_client on device removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_remove_one() and isert_free_np() on unbound conns
- [Critical] IB/isert: race between isert_connected_handler() and isert_remove_one() causes UAF
- [High] IB/isert: deadlock in isert_remove_one() on ISER_CONN_INIT connections
- [High] IB/isert: deadlock in device removal on racing or init-state connections
--

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

IB/isert: tear connections down from an ib_client on device removal

This commit registers an ib_client for ib_isert to tear down connections and
wait for the last isert_device reference during device removal instead of
waiting in the RDMA_CM_EVENT_DEVICE_REMOVAL callback. It also removes
isert_conn->dev_removed and isert_conn->rem_wait so that
isert_connect_release() is the sole place that destroys the cm_id and frees
isert_conn.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..35e4b9c917bd6 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -625,13 +637,10 @@ isert_np_cma_handler(struct isert_np *isert_np,
>  	return -1;
>  }
>  
> -static int
> -isert_disconnected_handler(struct rdma_cm_id *cma_id,
> -			   enum rdma_cm_event_type event)
> +/* Must be called with isert_conn->mutex held. */
> +static void
> +isert_teardown_conn(struct isert_conn *isert_conn)
>  {
> -	struct isert_conn *isert_conn = cma_id->qp->qp_context;
> -
> -	mutex_lock(&isert_conn->mutex);
>  	switch (isert_conn->state) {
>  	case ISER_CONN_TERMINATING:
>  		break;
> @@ -648,6 +657,16 @@ isert_disconnected_handler(struct rdma_cm_id *cma_id,
>  		isert_warn("conn %p terminating in state %d\n",
>  			   isert_conn, isert_conn->state);
>  	}
> +}

[Severity: High]
What happens in isert_teardown_conn() when a connection is still in
ISER_CONN_INIT during device removal?

During IB device unregistration, disable_device() invokes ib_client->remove
callbacks in reverse registration order:

drivers/infiniband/core/device.c:disable_device() {
	...
	while (cid) {
		cid--;
		remove_client_context(device, cid);
	}
}

Because isert_ib_client is registered after cma_client and cm_client,
isert_remove_one() runs and blocks on wait_for_completion(&removal_comp)
before cma_remove_one() and cm_remove_one() are called.

If a connection has processed RDMA_CM_EVENT_CONNECT_REQUEST in
isert_connect_request(), it holds a reference on isert_device and is on
isert_conn_list in state ISER_CONN_INIT. When isert_remove_one() calls
isert_teardown_conn(), ISER_CONN_INIT hits the default branch and only logs a
warning without terminating the connection.

Because cma_remove_one() and cm_remove_one() have not run yet, the CM remains
active while isert_remove_one() waits on removal_comp. If the peer's RTU
arrives, RDMA_CM_EVENT_ESTABLISHED invokes isert_connected_handler():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn->state = ISER_CONN_UP;
	kref_get(&isert_conn->kref);
	mutex_unlock(&isert_conn->mutex);
}

Since isert_remove_one() already finished its single pass over
isert_conn_list and cma_remove_one() is blocked behind isert_remove_one(),
won't this connection remain in ISER_CONN_UP (or advance to ISER_CONN_BOUND /
ISER_CONN_FULL_FEATURE via isert_accept_np()) without anything ever tearing
it down, causing wait_for_completion(&removal_comp) in isert_remove_one() to
hang forever?

Similarly, on iWARP (where a failed passive connection in ISER_CONN_INIT
delivers IW_CM_EVENT_CLOSE -> RDMA_CM_EVENT_DISCONNECTED rather than
RDMA_CM_EVENT_UNREACHABLE) or if RDMA_CM_EVENT_DEVICE_REMOVAL reaches an
ISER_CONN_INIT connection (where cma_send_device_removal_put() sets
id_priv->state = RDMA_CM_DEVICE_REMOVAL so cma_ib_handler() drops subsequent
IB_CM_REP_ERROR / RDMA_CM_EVENT_UNREACHABLE events), won't
isert_disconnected_handler() -> isert_teardown_conn() ignore ISER_CONN_INIT
and leak the connection reference?

[ ... ]

> @@ -2689,8 +2701,60 @@ static struct iscsit_transport iser_target_transport = {
>  	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
>  };
>  
> +static void
> +isert_remove_one(struct ib_device *ib_device, void *client_data)
> +{
> +	DECLARE_COMPLETION_ONSTACK(removal_comp);
> +	struct isert_device *device, *found = NULL;
> +	struct isert_conn *isert_conn;
> +
> +	/*
> +	 * Not from the RDMA_CM_EVENT_DEVICE_REMOVAL callback: that one runs
> +	 * with the cm_id's handler mutex held, which a teardown already
> +	 * destroying that cm_id needs.  Same reason as
> +	 * commit f1d4ef7d8883 ("nvmet-rdma: register ib_client to not
> +	 * deadlock in device removal").
> +	 */
> +	mutex_lock(&device_list_mutex);
> +	list_for_each_entry(device, &device_list, dev_node) {
> +		if (device->ib_device != ib_device)
> +			continue;
> +		device->refcount++;
> +		device->removal_comp = &removal_comp;
> +		found = device;
> +		break;
> +	}
> +	mutex_unlock(&device_list_mutex);
> +
> +	if (!found)
> +		return;

[Severity: High]
Because isert_remove_one() runs before cma_remove_one(), listening RDMA CM
IDs (isert_np->cm_id and per-device child listeners) are still in
RDMA_CM_LISTEN and continue delivering RDMA_CM_EVENT_CONNECT_REQUEST until
cma_remove_one() destroys them.

In isert_connect_request(), isert_device_get() increments device->refcount
under device_list_mutex without checking whether removal is in progress, and
isert_conn is only added to isert_conn_list after allocating login buffers,
creating the QP/CQ, posting a recv WR, and calling isert_rdma_accept():

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_request() {
	...
	device = isert_device_get(cma_id);
	if (IS_ERR(device)) {
		ret = PTR_ERR(device);
		goto out;
	}
	isert_conn->device = device;
	...
	mutex_lock(&isert_conn_mutex);
	list_add_tail(&isert_conn->conn_list, &isert_conn_list);
	mutex_unlock(&isert_conn_mutex);
}

If isert_remove_one() scans isert_conn_list while isert_connect_request() is
between isert_device_get() and list_add_tail(&isert_conn->conn_list,
&isert_conn_list), or if a new RDMA_CM_EVENT_CONNECT_REQUEST arrives while
isert_remove_one() is blocked in wait_for_completion(&removal_comp), can the
new connection acquire a reference on found and be added to isert_conn_list
after isert_remove_one() has already finished iterating the list, causing
wait_for_completion(&removal_comp) to hang?

Also, if isert_remove_one() returns (either immediately when found is NULL or
after removal_comp completes) and a new RDMA_CM_EVENT_CONNECT_REQUEST arrives
before cma_remove_one() destroys the listener, won't isert_device_get()
allocate a new struct isert_device on the removing ib_device after
isert_remove_one() has already finished? When cma_remove_one() ->
cma_process_remove() then runs cma_send_device_removal_put() on that new
connection's cm_id, id_priv->state is set to RDMA_CM_DEVICE_REMOVAL and
isert_cma_handler() returns 0; if the connection is still in ISER_CONN_INIT,
cma_ib_handler() drops all subsequent non-timewait IB CM events:

drivers/infiniband/core/cma.c:cma_ib_handler() {
	...
	mutex_lock(&id_priv->handler_mutex);
	state = READ_ONCE(id_priv->state);
	if ((ib_event->event != IB_CM_TIMEWAIT_EXIT &&
	     state != RDMA_CM_CONNECT) ||
	    (ib_event->event == IB_CM_TIMEWAIT_EXIT &&
	     state != RDMA_CM_DISCONNECT))
		goto out;
}

Wouldn't that prevent rdma_destroy_id() from ever being called and deadlock
cma_process_remove() at wait_for_completion(&cma_dev->comp)?

> +
> +	mutex_lock(&isert_conn_mutex);
> +	list_for_each_entry(isert_conn, &isert_conn_list, conn_list) {
> +		if (isert_conn->device != found)
> +			continue;
> +
> +		isert_info("removing conn %p state %d\n",
> +			   isert_conn, isert_conn->state);
> +		mutex_lock(&isert_conn->mutex);
> +		isert_teardown_conn(isert_conn);
> +		mutex_unlock(&isert_conn->mutex);

[Severity: Critical]
Can isert_remove_one() race with isert_free_np() on unbound connections in
ISER_CONN_UP and corrupt drop_conn_list or double-free isert_conn?

When an iSCSI network portal is deleted, isert_free_np() moves connections
from isert_np->pending and isert_np->accepted onto a stack-local
drop_conn_list under isert_np->mutex, and then releases isert_np->mutex
before calling isert_connect_release() on each entry:

drivers/infiniband/ulp/isert/ib_isert.c:isert_free_np() {
	...
	mutex_lock(&isert_np->mutex);
	if (!list_empty(&isert_np->pending)) {
		...
		list_for_each_entry_safe(isert_conn, n,
					 &isert_np->pending,
					 node) {
			...
			list_move_tail(&isert_conn->node, &drop_conn_list);
		}
	}
	...
	mutex_unlock(&isert_np->mutex);

	list_for_each_entry_safe(isert_conn, n, &drop_conn_list, node) {
		list_del_init(&isert_conn->node);
		isert_connect_release(isert_conn);
	}
}

Because those connections remain on isert_conn_list until
isert_connect_release() acquires isert_conn_mutex and removes them, a
concurrent isert_remove_one() walking isert_conn_list can find a connection in
ISER_CONN_UP that isert_free_np() has already moved onto drop_conn_list.

When isert_remove_one() calls isert_teardown_conn() ->
isert_handle_unbound_conn(), it checks !list_empty(&isert_conn->node) under
isert_np->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_handle_unbound_conn() {
	...
	mutex_lock(&isert_np->mutex);
	if (!list_empty(&isert_conn->node)) {
		list_del_init(&isert_conn->node);
		isert_put_conn(isert_conn);
		queue_work(isert_release_wq, &isert_conn->release_work);
	}
	mutex_unlock(&isert_np->mutex);
}

Since isert_conn->node is still linked into drop_conn_list,
!list_empty(&isert_conn->node) is true. Doesn't isert_handle_unbound_conn()
then mutate drop_conn_list via list_del_init(&isert_conn->node) while
isert_free_np() is traversing it without isert_np->mutex, and queue
isert_conn->release_work so that both isert_free_np() and
isert_release_work() call isert_connect_release(isert_conn)?

[Severity: Critical]
Could calling isert_teardown_conn() from isert_remove_one() without the
cm_id's handler_mutex also race with isert_connected_handler() and cause a
use-after-free?

Previously, both isert_connected_handler() and isert_disconnected_handler()
were called from isert_cma_handler() under the cm_id's handler_mutex. In
isert_connected_handler(), isert_conn->mutex is released after setting
ISER_CONN_UP and before acquiring isert_np->mutex to move isert_conn->node
from isert_np->accepted to isert_np->pending:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	mutex_lock(&isert_conn->mutex);
	isert_conn->state = ISER_CONN_UP;
	kref_get(&isert_conn->kref);
	mutex_unlock(&isert_conn->mutex);

	mutex_lock(&isert_np->mutex);
	list_move_tail(&isert_conn->node, &isert_np->pending);
	mutex_unlock(&isert_np->mutex);

	isert_info("np %p: Allow accept_np to continue\n", isert_np);
	up(&isert_np->sem);
}

If isert_remove_one() acquires isert_conn->mutex in that window between
mutex_unlock(&isert_conn->mutex) and mutex_lock(&isert_np->mutex),
isert_teardown_conn() sees state == ISER_CONN_UP, sets state to
ISER_CONN_TERMINATING, and calls isert_handle_unbound_conn(isert_conn).
Because isert_conn->node is still on isert_np->accepted,
isert_handle_unbound_conn() removes isert_conn->node with list_del_init(),
drops a kref via isert_put_conn(), and queues isert_conn->release_work.

When isert_connected_handler() then acquires isert_np->mutex, won't
list_move_tail(&isert_conn->node, &isert_np->pending) re-insert the
terminating connection onto isert_np->pending and wake isert_np->sem while
isert_release_work() drops the last kref and frees isert_conn in
isert_connect_release()?

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