Re: [PATCH v3 32/57] scsi: ips: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling scsi_done() before unmapping DMA causes a potential use-after-free of the scsi_cmnd structure.
- [High] Incorrect waitlist traversal causes premature loop termination and a potential use-after-free on `struct scsi_cmnd` if command submission fails.
- [High] Memory allocations use GFP_KERNEL in atomic contexts (while holding a spinlock or in a hardirq handler), which can sleep and cause a kernel panic.
- [High] Process-context functions acquire the SCSI host_lock using spin_lock() instead of spin_lock_irqsave(), leaving them vulnerable to deadlocks from the interrupt handler.
- [High] ips_wait() busy-waits with udelay(1000) for up to 60 seconds with hardware interrupts disabled, leading to a hard lockup.
- [High] Using BUG_ON() for scsi_dma_map() failures causes an unnecessary kernel panic.
--

--- Patch [32]: [PATCH v3 32/57] scsi: ips: Prepare for enabling lock context analysis ---
commit 3f51682bbdf047060a964b17e4ca87b13e0de2ad
Author: Bart Van Assche <bvanassche@acm.org>
scsi: ips: Prepare for enabling lock context analysis

This commit annotates ips_next() with __context_unsafe to indicate it
performs conditional locking. This prepares the driver for context lock
analysis.

> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index 41ed73966a48c..5ef70f058ecd6 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -2506,6 +2506,7 @@ ips_hainit(ips_ha_t * ha)
>  /****************************************************************************/
>  static void
>  ips_next(ips_ha_t * ha, int intr)
> +	__context_unsafe(conditional locking)
>  {
>  	ips_scb_t *scb;

[Severity: High]
This isn't a bug introduced by this patch, but does calling scsi_done()
before unmapping the DMA cause a potential use-after-free?

In ips_next() and ips_done(), the command is completed before the SCB is
freed:

drivers/scsi/ips.c:ips_next() {
    ...
    case IPS_FAILURE:
        if (scb->scsi_cmd) {
            scb->scsi_cmd->result = DID_ERROR << 16;
            scsi_done(scb->scsi_cmd);
        }

        ips_freescb(ha, scb);
        break;
    ...
}

Because scsi_done() hands ownership of the command to the block layer,
the block layer might asynchronously complete and free or recycle the
struct scsi_cmnd. Could ips_freescb() then dereference a freed pointer
when it subsequently calls scsi_dma_unmap(scb->scsi_cmd)?


[Severity: High]
This is a pre-existing issue, but does the waitlist traversal in
ips_next() prematurely terminate the loop and risk a use-after-free?

The list linkage is destroyed by explicitly clearing host_scribble:

drivers/scsi/ips.c:ips_next() {
    ...
    SC = ips_removeq_wait(&ha->scb_waitlist, q);
    ...
    SC->host_scribble = NULL;
    ...
}

If command submission fails or completes synchronously, scsi_done() is
invoked on the command, passing ownership of the command (aliased by p)
to the block layer. Could the loop then execute the following iteration
step using potentially freed memory?

drivers/scsi/ips.c:ips_next() {
    ...
    p = (struct scsi_cmnd *) p->host_scribble;
    ...
}

Furthermore, since host_scribble was set to NULL, would p become NULL
and terminate the dispatch loop early, starving any remaining queued
commands?


[Severity: High]
This isn't a bug introduced by this patch, but do the memory allocations
use GFP_KERNEL in atomic contexts?

When ips_next() processes passthru commands, it holds the host_lock
spinlock. If called from ips_queue_lck via IPS_INTR_IORL, it runs with
interrupts disabled and the lock is not dropped.

During this path, functions like ips_alloc_passthru_buffer() and
ips_flash_copperhead() perform allocations using GFP_KERNEL:

drivers/scsi/ips.c:ips_alloc_passthru_buffer() {
    ...
    bigger_buf = dma_alloc_coherent(&ha->pcidev->dev, length,
                                    &dma_busaddr, GFP_KERNEL);
    ...
}

Since GFP_KERNEL allows blocking, can this sleep in an atomic context
and trigger a kernel panic?


[Severity: High]
This is a pre-existing issue, but is there a deadlock vulnerability
between process-context functions and the interrupt handler?

The hardware interrupt handler do_ipsintr() runs in hardirq context and
acquires the lock using spin_lock(). Meanwhile, process-context
functions like ips_eh_abort() also acquire it without disabling local
interrupts:

drivers/scsi/ips.c:ips_eh_abort() {
    ...
    spin_lock(host->host_lock);
    ...
}

If an interrupt fires on the same CPU while the scsi_eh thread holds the
lock, will the interrupt handler attempt to acquire the lock and cause
an AB-BA self-deadlock?


[Severity: High]
This isn't a bug introduced by this patch, but does ips_wait()
busy-wait for too long with hardware interrupts disabled?

In ips_eh_reset(), hardware interrupts are completely disabled on the
local CPU:

drivers/scsi/ips.c:ips_eh_reset() {
    ...
    spin_lock_irq(SC->device->host->host_lock);
    rc = __ips_eh_reset(SC);
    ...
}

This triggers a flush that eventually falls back to a polling loop in
ips_wait(), which calls udelay(1000) up to 60,000 times:

drivers/scsi/ips.c:ips_wait() {
    ...
    /* This looks like a very evil loop, but it only does this during start-up */
    udelay(1000);
    time--;
    ...
}

Could looping for up to 60 seconds with interrupts disabled violate
latency requirements and trigger the kernel's hard lockup watchdog?


[Severity: High]
This is a pre-existing issue, but does using BUG_ON() for DMA mapping
failures cause an unnecessary kernel panic?

In ips_next(), a mapping failure triggers a system panic:

drivers/scsi/ips.c:ips_next() {
    ...
    scb->sg_count = scsi_dma_map(SC);
    BUG_ON(scb->sg_count < 0);
    ...
}

Since scsi_dma_map() can return negative values like -ENOMEM when IOMMU
mappings or DMA resources are exhausted, would it be better to gracefully
fail the command or propagate the error rather than intentionally crashing
the system?

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