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_t=
ask().

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 endia=
nness 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_m=
ain.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 initial=
ize
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 =3D (struct iscsi_logout_rsp *)task->hdr;
    hdr->opcode =3D ISCSI_OP_LOGOUT_RSP;
    ...
    hdr->itt =3D 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 =3D be32_to_cpu(hdr->statsn) + 1.

Can this overwrite the target status sequence number with the initiator com=
mand
sequence number, leading to immediate connection state corruption and break=
ing
iSCSI sequence numbering?

This same issue also appears in be_complete_tmf() and be_complete_nopin_res=
p().


[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 =3D (struct iscsi_logout_rsp *)task->hdr;
    ...
    hdr->t2wait =3D 5;
    ...
}

The driver assigns the native integer 5 to hdr->t2wait (a __be16 field) wit=
hout
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 befo=
re
attempting task reassignment during logout recovery?

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

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/cover.1785451174.gi=
[email protected]?part=3D15