Re: [PATCH v3 1/2] 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 2 potential issue(s) to consider:
- [Critical] IB/isert: race between isert_free_np() and isert_connected_handler()
- [Critical] IB/isert: race between isert_free_np() and isert_connect_error()
--

commit a36a7ac6c22cec858896a9509524018f8c67a5b1
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 take connections off
isert_np->pending and isert_np->accepted one at a time under
isert_np->mutex rather than moving them onto a local list, so that
isert_conn->node is empty when isert_np->mutex is dropped and
isert_handle_unbound_conn() does not unlink and release them a second time.

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index efa8d7b7844c9..5371bba71ba3c 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -2479,12 +2479,29 @@ isert_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
>  	return 0;
>  }
>  
> +static struct isert_conn *
> +isert_np_take_conn(struct isert_np *isert_np)
> +{
> +	struct isert_conn *isert_conn;
> +
> +	lockdep_assert_held(&isert_np->mutex);
> +
> +	isert_conn = list_first_entry_or_null(&isert_np->pending,
> +					      struct isert_conn, node);
> +	if (!isert_conn)
> +		isert_conn = list_first_entry_or_null(&isert_np->accepted,
> +						      struct isert_conn, node);
> +	if (isert_conn)
> +		list_del_init(&isert_conn->node);
> +
> +	return isert_conn;
> +}

[Severity: Critical]
Can isert_np_take_conn() race with isert_connect_error() when a connection on
isert_np->accepted fails concurrently with isert_free_np()?

In isert_free_np(), rdma_destroy_id(isert_np->cm_id) only destroys the
listening cm_id, leaving child connection cm_ids created by
isert_connect_request() active on isert_np->accepted with kref == 1.

If RDMA_CM_EVENT_REJECTED, RDMA_CM_EVENT_UNREACHABLE, or
RDMA_CM_EVENT_CONNECT_ERROR fires on a child cm_id,
cma_ib_handler()->isert_cma_handler()->isert_connect_error() runs under
id_priv->handler_mutex and calls ib_drain_qp() before taking isert_np->mutex:

drivers/infiniband/ulp/isert/ib_isert.c:isert_connect_error() {
	struct isert_conn *isert_conn = cma_id->qp->qp_context;
	struct isert_np *isert_np = cma_id->context;

	ib_drain_qp(isert_conn->qp);

	mutex_lock(&isert_np->mutex);
	list_del_init(&isert_conn->node);
	mutex_unlock(&isert_np->mutex);
	isert_conn->cm_id = NULL;
	isert_put_conn(isert_conn);

	return -1;
}

If isert_free_np() takes that connection via isert_np_take_conn() while
isert_connect_error() is in ib_drain_qp(), isert_free_np() drops
isert_np->mutex and calls isert_connect_release(isert_conn), which blocks in
rdma_destroy_id(isert_conn->cm_id) waiting for id_priv->handler_mutex.

When ib_drain_qp() finishes, isert_connect_error() does not check
!list_empty(&isert_conn->node) under isert_np->mutex. Would it then call
isert_put_conn(isert_conn) (dropping kref from 1 to 0 and running
isert_connect_release(isert_conn) -> kfree(isert_conn)) and return -1 to
destroy the cm_id while isert_free_np() is concurrently releasing the same
isert_conn?

> +
>  static void
>  isert_free_np(struct iscsi_np *np)
>  {
>  	struct isert_np *isert_np = np->np_context;
> -	struct isert_conn *isert_conn, *n;
> -	LIST_HEAD(drop_conn_list);
> +	struct isert_conn *isert_conn;
>  
>  	if (isert_np->cm_id)
>  		rdma_destroy_id(isert_np->cm_id);
> @@ -2497,31 +2514,22 @@ isert_free_np(struct iscsi_np *np)
>  	 * up in accepted and pending lists.
>  	 */
>  	mutex_lock(&isert_np->mutex);
> -	if (!list_empty(&isert_np->pending)) {
> +	if (!list_empty(&isert_np->pending))
>  		isert_info("Still have isert pending connections\n");
> -		list_for_each_entry_safe(isert_conn, n,
> -					 &isert_np->pending,
> -					 node) {
> -			isert_info("cleaning isert_conn %p state (%d)\n",
> -				   isert_conn, isert_conn->state);
> -			list_move_tail(&isert_conn->node, &drop_conn_list);
> -		}
> -	}
> -
> -	if (!list_empty(&isert_np->accepted)) {
> +	if (!list_empty(&isert_np->accepted))
>  		isert_info("Still have isert accepted connections\n");
> -		list_for_each_entry_safe(isert_conn, n,
> -					 &isert_np->accepted,
> -					 node) {
> -			isert_info("cleaning isert_conn %p state (%d)\n",
> -				   isert_conn, isert_conn->state);
> -			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);
> +	for (;;) {
> +		mutex_lock(&isert_np->mutex);
> +		isert_conn = isert_np_take_conn(isert_np);
> +		mutex_unlock(&isert_np->mutex);
> +
> +		if (!isert_conn)
> +			break;
> +
> +		isert_info("cleaning isert_conn %p state (%d)\n",
> +			   isert_conn, isert_conn->state);
>  		isert_connect_release(isert_conn);
>  	}

[Severity: Critical]
What happens if RDMA_CM_EVENT_ESTABLISHED fires on a connection just after
isert_np_take_conn() pops it from isert_np->accepted and drops
isert_np->mutex?

While isert_free_np()->isert_connect_release()->rdma_destroy_id() waits for
id_priv->handler_mutex, cma_ib_handler()->isert_cma_handler()->
isert_connected_handler() runs and moves isert_conn->node onto
isert_np->pending without checking !list_empty(&isert_conn->node):

drivers/infiniband/ulp/isert/ib_isert.c:isert_connected_handler() {
	...
	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);
}

Once isert_connected_handler() returns and drops id_priv->handler_mutex,
isert_connect_release() finishes and frees isert_conn via kfree(isert_conn)
while isert_conn->node is linked on isert_np->pending.

On the next iteration of the for (;;) loop in isert_free_np(), wouldn't
isert_np_take_conn() pop that already-freed isert_conn from isert_np->pending
and call isert_connect_release(isert_conn) a second time?

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