Re: [PATCH v3 05/57] scsi: BusLogic: Introduce two local variables

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver incorrectly uses spin_unlock_irq() and spin_lock_irq() to temporarily drop the host lock in paths protected by spin_lock_irqsave() or inside hardirq handlers, unconditionally enabling local interrupts and violating IRQ context guarantees.
--

--- Patch [5]: [PATCH v3 05/57] scsi: BusLogic: Introduce two local variables ---
commit 0f5e462089dbe7ff33a0eecd6a02a423a0f2c9ec
Author: Bart Van Assche <bvanassche@acm.org>

scsi: BusLogic: Introduce two local variables

The patch introduces two local variables to prepare for enabling lock context
analysis. No functionality has been changed.

> diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
> index 5304d2febd63e..f2a6ed710c770 100644
> --- a/drivers/scsi/BusLogic.c
> +++ b/drivers/scsi/BusLogic.c

[ ... ]

> @@ -2915,9 +2915,9 @@ static enum scsi_qc_status blogic_qcmd_lck(struct scsi_cmnd *command)
>  	 */
>  	ccb = blogic_alloc_ccb(adapter);
>  	if (ccb == NULL) {
> -		spin_unlock_irq(adapter->scsi_host->host_lock);
> +		spin_unlock_irq(shost->host_lock);

[Severity: High]
This isn't a bug introduced by this patch, but does using spin_unlock_irq()
here unconditionally enable interrupts while we are inside a
spin_lock_irqsave() block?

Since blogic_qcmd_lck() is wrapped by DEF_SCSI_QCMD() which acquires the lock
with spin_lock_irqsave(), it appears dropping the lock with spin_unlock_irq()
will unexpectedly enable local interrupts and corrupt the IRQ state context.

This same pattern also appears to exist in blogic_resetadapter() when called
from the hardirq handler blogic_inthandler() during adapter internal errors.

Could this lead to unexpected nested interrupts or IRQ storms on error paths?

>  		blogic_delay(1);
> -		spin_lock_irq(adapter->scsi_host->host_lock);
> +		spin_lock_irq(shost->host_lock);
>  		ccb = blogic_alloc_ccb(adapter);
>  		if (ccb == NULL) {
>  			command->result = DID_ERROR << 16;

[ ... ]

> @@ -3062,10 +3062,10 @@ static enum scsi_qc_status blogic_qcmd_lck(struct scsi_cmnd *command)
>  		   be initiated soon.
>  		 */
>  		if (!blogic_write_outbox(adapter, BLOGIC_MBOX_START, ccb)) {
> -			spin_unlock_irq(adapter->scsi_host->host_lock);
> +			spin_unlock_irq(shost->host_lock);

[Severity: High]
This is also a pre-existing issue, but won't this spin_unlock_irq() call
similarly enable local interrupts unexpectedly for the same reasons as above?

>  			blogic_warn("Unable to write Outgoing Mailbox - Pausing for 1 second\n", adapter);
>  			blogic_delay(1);
> -			spin_lock_irq(adapter->scsi_host->host_lock);
> +			spin_lock_irq(shost->host_lock);
>  			if (!blogic_write_outbox(adapter, BLOGIC_MBOX_START,
>  						ccb)) {
>  				blogic_warn("Still unable to write Outgoing Mailbox - Host Adapter Dead?\n", adapter);

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