Re: [PATCH v2] scsi: libiscsi_tcp: bound SCSI Response data segment to the connection buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the iSCSI TCP receive path acquires `session->back_lock` using plain `spin_lock()`, which can deadlock if a softirq preempts the workqueue.
--

--- Patch [1]: [PATCH v2] scsi: libiscsi_tcp: bound SCSI Response data segment to the connection buffer ---
commit 03e41264108be30b9b653aa5ed7bc2fa484238e9
Author: HyeongJun An <sammiee5311@gmail.com>

scsi: libiscsi_tcp: bound SCSI Response data segment to the connection buffer
    
This commit bounds the data segment of SCSI Command Responses to the connection
buffer in iscsi_tcp_hdr_dissect(). It groups the opcode with LOGIN_RSP and
other response types to apply the existing bounds check.

> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index e90805ba868fb..7223bb18b0480 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -752,13 +752,6 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
>  		rc = __iscsi_complete_pdu(conn, hdr, NULL, 0);
>  		spin_unlock(&conn->session->back_lock);
>  		break;
> -	case ISCSI_OP_SCSI_CMD_RSP:
> -		if (tcp_conn->in.datalen) {
> -			iscsi_tcp_data_recv_prep(tcp_conn);
> -			return 0;
> -		}
> -		rc = iscsi_complete_pdu(conn, hdr, NULL, 0);

[Severity: High]
This isn't a bug introduced by this patch, but can the plain spin_lock()
used here and in the receive path cause a self-deadlock?

If the iscsi_recv_from_iscsi_q module parameter is enabled, packet reception
is routed to the workqueue which runs in process context:

drivers/scsi/iscsi_tcp.c:iscsi_sw_tcp_recv_data_work() {
    ...
    lock_sock(sk);
    iscsi_sw_tcp_recv_data(conn);
    release_sock(sk);
}

When iscsi_tcp_hdr_dissect() handles ISCSI_OP_SCSI_DATA_IN, it takes
the back_lock using a plain spin_lock():

drivers/scsi/libiscsi_tcp.c:iscsi_tcp_hdr_dissect() {
    ...
    case ISCSI_OP_SCSI_DATA_IN:
        spin_lock(&conn->session->back_lock);
    ...
}

Similarly, the iscsi_complete_pdu() function being removed from this specific
case statement, but still called for these operations, takes back_lock with
a plain spin_lock():

drivers/scsi/libiscsi.c:iscsi_complete_pdu() {
    ...
    spin_lock(&conn->session->back_lock);
    rc = __iscsi_complete_pdu(conn, hdr, data, datalen);
    spin_unlock(&conn->session->back_lock);
    ...
}

If a softirq (like a block completion or timer) interrupts the workqueue
thread while this lock is held, and the softirq attempts to acquire the same
lock using spin_lock_bh() (e.g., in the iscsi_queuecommand error path),
would it spin forever?

Does the workqueue handler need to disable bottom halves before entering the
receive path?

> -		break;
>  	case ISCSI_OP_R2T:
>  		if (ahslen) {
>  			rc = ISCSI_ERR_AHSLEN;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716065848.1653431-1-sammiee5311@gmail.com?part=1