Re: [PATCH v7 3/6] 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 1 potential issue(s) to consider:
- [High] IB/isert: NULL cm_id dereference in isert_qp_event_callback()
--

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

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

This patch registers an ib_client for isert to tear down device connections
from its remove callback instead of waiting and freeing connections inside the
RDMA_CM_EVENT_DEVICE_REMOVAL handler. It also adds reference counting for
isert_np and tracks connections on isert_conn_list under isert_conn_mutex.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 61b214012a70b..4812dabd93945 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c

[ ... ]

> @@ -661,20 +721,33 @@ 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;
> -	bool owned;
> +	bool owned = false, bound = false;
>  
> +	mutex_lock(&isert_conn->mutex);
>  	mutex_lock(&isert_np->mutex);
> -	owned = !list_empty(&isert_conn->node);
> -	list_del_init(&isert_conn->node);
> +	if (list_empty(&isert_conn->node)) {
> +		bound = isert_conn->state == ISER_CONN_BOUND ||
> +			isert_conn->state == ISER_CONN_FULL_FEATURE;
> +	} else {
> +		owned = true;
> +		list_del_init(&isert_conn->node);
> +	}
>  	mutex_unlock(&isert_np->mutex);
>  
> +	if (owned) {
> +		isert_conn->cm_id = NULL;
> +	} else if (bound) {
> +		isert_conn_terminate(isert_conn);
> +		iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
> +	}
> +	mutex_unlock(&isert_conn->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);
>  
>  	return -1;

[Severity: High]
Can clearing isert_conn->cm_id before draining and destroying the QP cause a
NULL pointer dereference in isert_qp_event_callback()?

During passive connection setup, isert_connect_request() creates
isert_conn->qp with isert_qp_event_callback() as its event handler and calls
isert_rdma_accept() -> rdma_accept(), which transitions the QP to RTR/RTS.

When RDMA_CM_EVENT_UNREACHABLE, RDMA_CM_EVENT_REJECTED, or
RDMA_CM_EVENT_CONNECT_ERROR invokes isert_connect_error() while the connection
is still on isert_np->accepted or isert_np->pending (owned is true),
isert_conn->cm_id is now set to NULL before ib_drain_qp(isert_conn->qp) runs
and before the QP is destroyed. In addition, if isert_remove_one() holds a
reference on isert_conn via kref_get_unless_zero(), isert_put_conn() in
isert_connect_error() will not drop kref to zero, leaving isert_conn->qp alive
with isert_conn->cm_id == NULL until isert_remove_one() drops its reference.

On RDMA_CM_EVENT_UNREACHABLE, cma does not transition the QP to IB_QPS_ERR
before invoking the handler, and drivers such as mlx4 and mlx5 dispatch
IB_EVENT_COMM_EST asynchronously on a workqueue that is not synchronized by
ib_drain_qp(). If IB_EVENT_COMM_EST runs in isert_qp_event_callback() during
this window:

drivers/infiniband/ulp/isert/ib_isert.c:isert_qp_event_callback() {
	...
	switch (e->event) {
	case IB_EVENT_COMM_EST:
		rdma_notify(isert_conn->cm_id, IB_EVENT_COMM_EST);
		break;
	...
}

rdma_notify() will be called with a NULL cm_id and dereference
id_priv->cm_id.ib at a NULL base pointer.

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