Re: [PATCH -next 1/2] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces an endpoint (`struct iscsi_endpoint`) reference leak by unconditionally clearing `ISCSI_CLS_CONN_BIT_CLEANUP` in `iscsi_if_stop_conn()` during a `STOP_CONN_TERM`, causing the subsequent endpoint disconnect to incorrectly skip releasing the workqueue's reference.
--

--- Patch [1]: [PATCH -next 1/2] scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn() ---
commit 15cab28507a9da46dab7d098d970770bae480083
Author: Ye Bin <[email protected]>

scsi: iscsi: fix NULL pointer dereference in iscsi_sw_tcp_release_conn()

This commit addresses a NULL pointer dereference in iscsi_sw_tcp_release_conn()
by ensuring the ISCSI_CLS_CONN_BIT_CLEANUP bit is set during STOP_CONN_TERM.
This prevents concurrent execution of iscsi_stop_conn().

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 8aa76f813bcdb..aea319c1e72fe 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2278,35 +2280,36 @@ static int iscsi_if_stop_conn(struct iscsi_cls_conn *conn, int flag)

[ ... ]

> -		/*
> -		 * Only clear for recovery to avoid extra cleanup runs during
> -		 * termination.
> -		 */
> -		spin_lock_irq(&conn->lock);
> -		clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> -		spin_unlock_irq(&conn->lock);
> -	}
> +	} else {
> +		iscsi_stop_conn(conn, flag);
> +	}
> +
> +	spin_lock_irq(&conn->lock);
> +	clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
> +	spin_unlock_irq(&conn->lock);

[Severity: High]
Does this unconditionally clear ISCSI_CLS_CONN_BIT_CLEANUP during
STOP_CONN_TERM?

The original code avoided clearing this bit during termination, as the removed
comment explicitly noted.

If a connection error queues iscsi_cleanup_conn_work_fn(), which unbinds the
endpoint (setting conn->ep = NULL), and then userspace initiates termination
via iscsi_if_stop_conn(conn, STOP_CONN_TERM), iscsi_if_disconnect_bound_ep()
won't be called here because conn->ep is NULL.

When this bit is subsequently cleared unconditionally, and userspace later issues
an event to disconnect the endpoint, will iscsi_if_disconnect_bound_ep() skip
releasing the workqueue's reference?

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_disconnect_bound_ep() {
    ...
	if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
		spin_unlock_irq(&conn->lock);
		iscsi_ep_disconnect(conn, is_active);
	} else {
		...
		iscsi_put_endpoint(ep);
    ...
}

Could this cause a resource leak of struct iscsi_endpoint over time?

> +
>  	ISCSI_DBG_TRANS_CONN(conn, "iscsi if conn stop done.\n");
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1