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

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.