Re: [PATCH] scsi: iscsi_tcp: Fix null-pointer dereference in iscsi_sw_tcp_conn_restore_callbacks

[Mike Christie]

On 8/5/26 1:53 AM, Jiayuan Liang wrote:
> A null-pointer dereference can occur in iscsi_sw_tcp_conn_restore_callbacks() due to a race condition leading to concurrent/re-entrant invocations of iscsi_sw_tcp_release_conn(). Specifically, the re-entrancy can be triggered under the following
>
>
> A null-pointer dereference can occur in
> iscsi_sw_tcp_conn_restore_callbacks() due to a race condition
> leading to concurrent/re-entrant invocations of
> iscsi_sw_tcp_release_conn().
>
> Specifically, the re-entrancy can be triggered under the
> following scenario:
>
> 1. The iSCSI client initiates a logout, actively stopping the
> connection via:
> iscsi_if_stop_conn()
> -> iscsi_stop_conn(..., STOP_CONN_TERM)
> -> cancel_work_sync(&conn->cleanup_work)
> -> iscsi_sw_tcp_release_conn()
>
> 2. Simultaneously, a server disconnect triggers a heartbeat
> timeout on the client side, executing the timeout path:
> iscsi_check_transport_timeouts()
> -> iscsi_conn_failure()
> -> iscsi_conn_error_event()
> -> queue_work(..., &conn->cleanup_work)
>
> This schedules iscsi_cleanup_conn_work_fn(), which calls:
> iscsi_cleanup_conn_work_fn()
> -> iscsi_stop_conn(..., STOP_CONN_RECOVER)
> -> iscsi_sw_tcp_release_conn()
>
> If these two paths execute concurrently, iscsi_sw_tcp_release_conn()
> is re-entered. Since the first invocation releases the socket and
> sets tcp_sw_conn->sock to NULL, the subsequent re-entrant
> invocation in iscsi_sw_tcp_conn_restore_callbacks() attempts to
> dereference the NULL pointer at `tcp_sw_conn->sock->sk`, resulting
> in a kernel panic (Oops):
>
We don't want to allow iscsi_cleanup_conn_work_fn to run after a
termination and we don't want to allow iscsi_cleanup_conn_work_fn
and iscsi_if_stop_conn to run concurrently.

Can we have iscsi_if_stop_conn hold the conn->ep_mutex when calling
iscsi_stop_conn. iscsi_cleanup_conn_work_fn would then have a check
for for if the conn->state was ISCSI_CONN_DOWN and if so not call
iscsi_stop_conn.

I think iscsi_if_stop_conn could also call cancel_work_sync
after calling iscsi_stop_conn for both the STOP_CONN_TERM and
STOP_CONN_RECOVER cases instead of calling it for the
STOP_CONN_TERM before calling iscsi_stop_conn.