Re: [PATCH v4 1/3] IB/isert: do not publish connections being released on a second list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] IB/isert: missing ib_drain_qp() in isert_free_np() and isert_connect_error()
--

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

IB/isert: do not publish connections being released on a second list

This commit changes isert_free_np() to unlink connections from
isert_np->pending and isert_np->accepted one at a time rather than moving
them onto a local list, and updates the CM handlers to check whether
isert_conn->node is already empty under isert_np->mutex before acting on
the connection.

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

[ ... ]

> @@ -658,12 +663,19 @@ isert_connect_error(struct rdma_cm_id *cma_id)
>  {
>  	struct isert_conn *isert_conn = cma_id->qp->qp_context;
>  	struct isert_np *isert_np = cma_id->context;
> -
> -	ib_drain_qp(isert_conn->qp);
> +	bool owned;
>  
>  	mutex_lock(&isert_np->mutex);
> +	owned = !list_empty(&isert_conn->node);
>  	list_del_init(&isert_conn->node);
>  	mutex_unlock(&isert_np->mutex);
> +
> +	if (!owned) {
> +		isert_info("conn %p is being released already\n", isert_conn);
> +		return 0;
> +	}
> +
> +	ib_drain_qp(isert_conn->qp);
>  	isert_conn->cm_id = NULL;
>  	isert_put_conn(isert_conn);
>  
> @@ -2479,12 +2491,33 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)

[ ... ]

> @@ -2497,32 +2530,25 @@ isert_free_np(struct iscsi_np *np)

[ ... ]

> +	for (;;) {
> +		mutex_lock(&isert_np->mutex);
> +		isert_conn = isert_np_take_conn(isert_np, &promoted);
> +		mutex_unlock(&isert_np->mutex);
> +
> +		if (!isert_conn)
> +			break;
> +
> +		isert_info("cleaning isert_conn %p state (%d)\n",
> +			   isert_conn, isert_conn->state);
> +		if (promoted)
> +			isert_put_conn(isert_conn);
> +		isert_put_conn(isert_conn);
>  	}

[Severity: Critical]
Can skipping ib_drain_qp(isert_conn->qp) when isert_free_np() takes ownership
of the connection cause a use-after-free in isert_login_recv_done()?

During RDMA_CM_EVENT_CONNECT_REQUEST, isert_connect_request() allocates a QP
using a shared workqueue-polled CQ from ib_cq_pool_get(..., IB_POLL_WORKQUEUE),
posts the login receive WR with wr_cqe = &isert_conn->login_desc->rx_cqe
(callback isert_login_recv_done) via isert_login_post_recv(), and transitions
the QP to RTS in isert_rdma_accept() before adding isert_conn->node to
isert_np->accepted.

When an IB_CM_REJ_RECEIVED event (or a cma_rep_recv() failure) occurs,
cma_ib_handler() calls cma_modify_qp_err() before invoking
cma_cm_event_handler() -> isert_cma_handler() -> isert_connect_error(). That
QP state transition flushes the pre-posted login receive WR into the shared CQ
and queues cq->work (ib_cq_poll_work). Likewise, for a connection on
isert_np->accepted or isert_np->pending, the remote initiator may already have
sent a login PDU that completed into the shared CQ.

Previously, isert_connect_error() unconditionally called
ib_drain_qp(isert_conn->qp) at the start of the function (added in commit
cd52cb26e7ea ("IB/isert: Fix use-after-free in isert_connect_error")) before
taking isert_np->mutex. With this change, if isert_free_np() unlinks
isert_conn->node via isert_np_take_conn() first, isert_connect_error() sees
!owned and returns 0 without draining the QP, and neither isert_free_np() nor
isert_connect_release() calls ib_drain_qp(isert_conn->qp):

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_release() {
	...
	if (isert_conn->cm_id &&
	    !isert_conn->dev_removed)
		rdma_destroy_id(isert_conn->cm_id);

	if (isert_conn->qp)
		isert_destroy_qp(isert_conn);

	isert_unmap_tx_desc(&isert_conn->login_tx_desc, device->ib_device);

	if (isert_conn->login_desc)
		isert_free_login_buf(isert_conn);
	...
	kfree(isert_conn);
}

Because isert_destroy_qp() releases the shared CQ via ib_cq_pool_put(), which
only decrements cq->cqe_used without canceling or flushing cq->work, can
ib_cq_poll_work concurrently invoke isert_login_recv_done() and dereference
wc->qp->qp_context, isert_conn->device, isert_conn->login_desc, and
isert_conn->mutex after isert_connect_release() has already freed them?

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