Re: [PATCH v3 15/57] scsi: be2iscsi: Prepare for enabling lock context analysis

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

New issues:
- [Low] The patch adds the __must_hold() lock context annotation to several task completion handlers but misses be_complete_io() and beiscsi_cleanup_task().

Pre-existing issues:
- [High] Synthesized response PDUs fail to initialize `statsn`, corrupting connection state.
- [Medium] Assignment of a native integer to a `__be16` field without endianness conversion.
--

--- Patch [15]: [PATCH v3 15/57] scsi: be2iscsi: Prepare for enabling lock context analysis ---
commit 4a36248898c49dd5d9e82e5e2120d9998eb707c9
Author: Bart Van Assche <[email protected]>

scsi: be2iscsi: Prepare for enabling lock context analysis

This commit prepares the be2iscsi driver for lock context analysis. It adds the
__must_hold() macro to document locking requirements on various completion
handlers.

> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index fd18d4d3d219b..289d9d18482c3 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -1185,6 +1185,7 @@ static void
>  be_complete_logout(struct beiscsi_conn *beiscsi_conn,
>  		    struct iscsi_task *task,
>  		    struct common_sol_cqe *csol_cqe)
> +	__must_hold(&beiscsi_conn->conn->session->back_lock)

[Severity: Low]
Does this patch miss annotating some other completion handlers?

For example, hwi_complete_cmd() calls be_complete_io() under the exact same
session->back_lock:

hwi_complete_cmd() {
    spin_lock_bh(&session->back_lock);
    ...
    be_complete_io(beiscsi_conn, task, &csol_cqe);
}

Should be_complete_io() and beiscsi_cleanup_task() also receive the
__must_hold() annotation to fully document the locking requirements for the
core I/O completion and cleanup paths?


[Severity: High]
This is a pre-existing issue, but synthesised response PDUs fail to initialize
statsn, which can corrupt the connection state.

In this function, the response PDU is constructed using the memory of the
original request PDU:

be_complete_logout() {
    hdr = (struct iscsi_logout_rsp *)task->hdr;
    hdr->opcode = ISCSI_OP_LOGOUT_RSP;
    ...
    hdr->itt = io_task->libiscsi_itt;
    __iscsi_complete_pdu(conn, (struct iscsi_hdr *)hdr, NULL, 0);
}

Because statsn in the response overlaps exactly with cmdsn in the request, and
statsn is left uninitialized, the response inherits the request's sequence
number. When libiscsi processes this fake PDU via __iscsi_complete_pdu(), it
updates conn->exp_statsn = be32_to_cpu(hdr->statsn) + 1.

Can this overwrite the target status sequence number with the initiator command
sequence number, leading to immediate connection state corruption and breaking
iSCSI sequence numbering?

This same issue also appears in be_complete_tmf() and be_complete_nopin_resp().


[Severity: Medium]
This is a pre-existing issue, but is a native integer assigned to a __be16
field without endianness conversion here?

be_complete_logout() {
    hdr = (struct iscsi_logout_rsp *)task->hdr;
    ...
    hdr->t2wait = 5;
    ...
}

The driver assigns the native integer 5 to hdr->t2wait (a __be16 field) without
using cpu_to_be16(). On little-endian architectures, this stores 0x05, 0x00.

When iscsid parses this via be16_to_cpu(), it interprets the value as 1280
seconds instead of 5. Can this cause the initiator to wait ~21 minutes before
attempting task reassignment during logout recovery?

>  {
>  	struct iscsi_logout_rsp *hdr;
>  	struct beiscsi_io_task *io_task = task->dd_data;
[ ... ]

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