Re: [PATCH v3 06/57] scsi: BusLogic: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The lock expressions used in the new `__must_hold` annotations do not syntactically match the lock variables used inside the functions and their callers, causing false positive context imbalance warnings in static analyzers.
- [Low] The patch misses several other functions in the driver that explicitly require the host lock, leaving the lock context annotations incomplete.

Pre-existing issues:
- [High] `spin_unlock_irq()` unconditionally enables interrupts in contexts where they must be preserved (e.g., inside `spin_lock_irqsave()` regions and hard IRQ handlers).
- [High] A failure in DMA mapping triggers a kernel panic via `BUG_ON()` rather than gracefully failing the command.
- [High] Missing memory barrier (`wmb()`) between CCB and Action Code writes in `blogic_write_outbox`.
--

--- Patch [6]: [PATCH v3 06/57] scsi: BusLogic: Prepare for enabling lock context analysis ---
commit 927e16987713b901aca16b28f9b8a4d54db270de
Author: Bart Van Assche <bvanassche@acm.org>

scsi: BusLogic: Prepare for enabling lock context analysis

This commit prepares the BusLogic SCSI driver for lock context analysis.
It adds the __must_hold() macro to document locking requirements for
blogic_qcmd_lck() and blogic_resetadapter().

> diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
> index f2a6ed710c770..dafec8b24e586 100644
> --- a/drivers/scsi/BusLogic.c
> +++ b/drivers/scsi/BusLogic.c
> @@ -2878,6 +2878,7 @@ static int blogic_hostreset(struct scsi_cmnd *SCpnt)
>  */
>  
>  static enum scsi_qc_status blogic_qcmd_lck(struct scsi_cmnd *command)
> +	__must_hold(command->device->host->host_lock)
>  {

[Severity: Medium]
Will this annotation cause false positive context imbalance warnings in static
analyzers like Sparse?

The annotation requires command->device->host->host_lock, but the function
body internally unlocks and locks shost->host_lock:

	spin_unlock_irq(shost->host_lock);
	blogic_delay(1);
	spin_lock_irq(shost->host_lock);

Since Sparse evaluates lock context expressions syntactically, will this
structural mismatch trigger the exact warnings this patch intends to prevent?

[Severity: High]
This isn't a bug introduced by this patch, but doesn't spin_unlock_irq()
unconditionally enable interrupts in contexts where they must be preserved?

Looking at blogic_qcmd_lck() in drivers/scsi/BusLogic.c:

	spin_unlock_irq(shost->host_lock);
	blogic_delay(1);
	spin_lock_irq(shost->host_lock);

Since blogic_qcmd_lck() is called from inside a region protected by
spin_lock_irqsave() via DEF_SCSI_QCMD, does this break the caller's IRQ state
by forcefully enabling interrupts?

[Severity: High]
This is a pre-existing issue, but does a DMA mapping failure here trigger a
kernel panic instead of gracefully failing the command?

drivers/scsi/BusLogic.c:blogic_qcmd_lck() {
    ...
	count = scsi_dma_map(command);
	BUG_ON(count < 0);
    ...
}

Under high memory pressure or extreme fragmentation, could scsi_dma_map()
return a negative error code and cause a Denial of Service?

>  	void (*comp_cb)(struct scsi_cmnd *) = scsi_done;
>  	struct blogic_adapter *adapter =

[ ... ]

> diff --git a/drivers/scsi/BusLogic.h b/drivers/scsi/BusLogic.h
> index 24697a5bedc8b..c152b93fb6ddb 100644
> --- a/drivers/scsi/BusLogic.h
> +++ b/drivers/scsi/BusLogic.h
> @@ -1279,7 +1279,8 @@ static int blogic_sdev_configure(struct scsi_device *,
>  				 struct queue_limits *lim);
>  static void blogic_qcompleted_ccb(struct blogic_ccb *);
>  static irqreturn_t blogic_inthandler(int, void *);
> -static int blogic_resetadapter(struct blogic_adapter *, bool hard_reset);
> +static int blogic_resetadapter(struct blogic_adapter *adapter, bool hard_reset)
> +	__must_hold(adapter->scsi_host->host_lock);

[Severity: Medium]
Will this syntactic mismatch also cause warnings with static analyzers?

The caller blogic_hostreset() acquires shost->host_lock:

	spin_lock_irq(shost->host_lock);
	blogic_inc_count(&stats->adapter_reset_req);
	rc = blogic_resetadapter(adapter, false);
	spin_unlock_irq(shost->host_lock);

But the annotation requires adapter->scsi_host->host_lock.  Could this
mismatch lead to analysis noise?

[Severity: Low]
Are there other functions in this driver that explicitly require the host
lock but missed this annotation?

Functions like blogic_alloc_ccb() and blogic_write_outbox() have header
comments explicitly stating "The Host Adapter's Lock should already have been
acquired by the caller":

static struct blogic_ccb *blogic_alloc_ccb(struct blogic_adapter *adapter)

static bool blogic_write_outbox(struct blogic_adapter *adapter,
		enum blogic_action action, struct blogic_ccb *ccb)

Would adding __must_hold() to these functions complete the static analysis
coverage?

[Severity: High]
This isn't a bug introduced by this patch, but does blogic_resetadapter()
forcefully enable hardware interrupts inside a hardirq context?

The interrupt handler blogic_inthandler() calls blogic_resetadapter() on
errors. Inside blogic_resetadapter():

	spin_unlock_irq(adapter->scsi_host->host_lock);
	blogic_delay(2);
	spin_lock_irq(adapter->scsi_host->host_lock);

Since this drops the lock using spin_unlock_irq() and busy-waits for 2
seconds, can this lead to hard IRQ re-entrancy, stack overflows, or potential
deadlocks?

[Severity: High]
This is a pre-existing issue, but is there a missing memory barrier in
blogic_write_outbox()?

drivers/scsi/BusLogic.c:blogic_write_outbox() {
    ...
		next_outbox->ccb = ccb->dma_handle;
		next_outbox->action = action;
		blogic_execmbox(adapter);
    ...
}

Because these assignments to memory polled by DMA are not protected by a
compiler or memory barrier, can a weakly-ordered CPU reorder the stores to
next_outbox->ccb and next_outbox->action?

If the Host Adapter asynchronously polls and observes the new action code
before the CCB address is fully committed to memory, could this cause the
hardware to execute invalid or stale CCB addresses?

>  static void blogic_msg(enum blogic_msglevel, char *, struct blogic_adapter *, ...);
>  static int __init blogic_setup(char *);

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