Re: [PATCH v3 56/57] scsi: core: Enable lock context analysis

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [High] Unconditional evaluation of `io_req->cb_arg->aborted_io_req->tgt->=
tgt_lock` causes a NULL pointer dereference for standard SCSI commands.
- [Medium] Falsely adding `__acquire()` macros to trick the static analyzer=
 instead of correctly annotating the function contract.
--

--- Patch [56]: [PATCH v3 56/57] scsi: core: Enable lock context analysis -=
--
commit 107a7992c0ed289ceb2e27bff45ba1eb5b8101fd
Author: Bart Van Assche <[email protected]>

scsi: core: Enable lock context analysis

Document which functions expect that shost->scan_mutex is held. Inform
the compiler about synchronization object aliases with __assume_ctx_lock().
Enable lock context analysis for the SCSI core and also for all drivers
in the drivers/scsi/ directory.

> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 842c254bb2269..a1fbdf4b0705a 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -14,6 +14,7 @@
>  # satisfy certain initialization assumptions in the SCSI layer.
>  # *!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!
> =20
> +CONTEXT_ANALYSIS :=3D y

[Severity: High]
Does enabling context analysis here cause a NULL pointer dereference in
bnx2fc_process_cq_compl()?

When context analysis is enabled, __assume_ctx_lock() forces the compiler
to evaluate its argument. For standard SCSI commands where io_req->cb_arg
is NULL, this expression in drivers/scsi/bnx2fc/bnx2fc_hwi.c will
dereference a NULL pointer on the CQ completion fast path:

	/* Tell the compiler that there is an alias for tgt->tgt_lock. */
	__assume_ctx_lock(&io_req->cb_arg->aborted_io_req->tgt->tgt_lock);


[Severity: Medium]
Are the __acquire() annotations in pqi_process_soft_reset() masking an
unbalanced unlock from the static analyzer?

By enabling context analysis, it looks like this exposes a mismatch in the
contract for pqi_ofa_quiesce_worker() in drivers/scsi/smartpqi/smartpqi_ini=
t.c.

The worker is incorrectly annotated as returning with the scan_mutex and
lun_reset_mutex held:

static void pqi_ofa_quiesce_worker(struct work_struct *work)
	__acquires(&quiesce_work_to_ctrl_info(work)->scan_mutex)
	__acquires(&quiesce_work_to_ctrl_info(work)->lun_reset_mutex)

However, the worker calls pqi_process_soft_reset(), which unconditionally
drops these locks in all execution paths.

It looks like fake __acquire() macros were added to pqi_process_soft_reset()
to trick the analyzer into ignoring the unbalanced unlocks:

	/* What guarantees that these mutexes are held here? */
	__acquire(&ctrl_info->lun_reset_mutex);
	__acquire(&ctrl_info->ofa_mutex);
	__acquire(&ctrl_info->scan_mutex);
	pqi_ctrl_ofa_done(ctrl_info);
	pqi_ofa_ctrl_unquiesce(ctrl_info);

Does this intentionally hide the dynamic lock release pattern from the stat=
ic
analyzer instead of correctly annotating the function contract?

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