Re: [PATCH v3 47/57] scsi: qla1280: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Incomplete `__must_hold` annotations on intermediate functions calling `qla1280_mailbox_command`.

Pre-existing issues:
- [High] Lost wakeups and stack pointer leak in `qla1280_poll()`.
--

--- Patch [47]: [PATCH v3 47/57] scsi: qla1280: Prepare for enabling lock context analysis ---
commit 1c9b404ec1f5e106acf48d1ff8e578d2279ecab4
Author: Bart Van Assche <bvanassche@acm.org>

scsi: qla1280: Prepare for enabling lock context analysis
    
Document locking requirements with __must_hold().

> diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
> index cdd6fe002c324..ebc878068684b 100644
> --- a/drivers/scsi/qla1280.c
> +++ b/drivers/scsi/qla1280.c

[ ... ]

> @@ -2413,6 +2425,7 @@ qla1280_nv_write(struct scsi_qla_host *ha, uint16_t data)
>   */
>  static int
>  qla1280_mailbox_command(struct scsi_qla_host *ha, uint8_t mr, uint16_t *mb)
> +	__must_hold(ha->host->host_lock)
>  {

[Severity: Medium]
Will static analysis tools emit false-positive lock context warnings for other
functions that call qla1280_mailbox_command() but lack the __must_hold()
annotation?

For example, intermediate functions like qla1280_set_target_parameters(),
qla1280_chip_diag(), qla1280_start_firmware(), qla1280_init_rings(), and
qla1280_nvram_config() call qla1280_mailbox_command() while the lock is
already held, but they are missing the annotation added to other callers
in this patch.

[ ... ]

> @@ -2538,6 +2551,7 @@ qla1280_poll(struct scsi_qla_host *ha)

[Severity: High]
This is a pre-existing issue, but does qla1280_poll() leak completed
SCSI commands on its stack-allocated done_q?

Looking at the qla1280_poll() implementation nearby, it allocates a local
list:

qla1280_poll(struct scsi_qla_host *ha)
{
    LIST_HEAD(done_q);
    ...
    if (data & RISC_INT)
        qla1280_isr(ha, &done_q);

    if (!list_empty(&done_q))
        qla1280_done(ha);
}

If qla1280_isr() populates this local done_q with completed commands, they
appear to be ignored when qla1280_done() is called, because qla1280_done()
only processes ha->done_q. When qla1280_poll() returns, the stack frame is
destroyed, permanently leaking these commands and leaving their list node
pointers aimed at invalid stack memory.

>   */
>  static int
>  qla1280_bus_reset(struct scsi_qla_host *ha, int bus)
> +	__must_hold(ha->host->host_lock)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785451174.git.bvanassche@acm.org?part=47