Re: [PATCH v3 49/57] scsi: qla4xxx: Enable lock context analysis

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of the hardware semaphore lock acquisition qla4_82xx_crb_win_lock() is completely ignored, leading to unprotected concurrent modification of the CRB window.
- [High] A potentially 16-minute long busy-wait loop in qla4_82xx_crb_win_lock() runs with local IRQs disabled, leading to a hard lockup panic.
- [High] The driver fails to check for timeout errors when acquiring hardware semaphore locks via qla4_82xx_rom_lock() and qla4_82xx_idc_lock().
- [High] The hardware window register is modified, but the protective hardware lock is dropped before performing I/O against the window, opening a TOCTOU race condition.
--

--- Patch [49]: [PATCH v3 49/57] scsi: qla4xxx: Enable lock context analysis ---
commit f4615e07e950d925c6341ed2210c4fa827b136dc
Author: Bart Van Assche <bvanassche@acm.org>

scsi: qla4xxx: Enable lock context analysis
    
This commit annotates the functions that perform conditional locking
with __no_context_analysis.

> diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c
> --- a/drivers/scsi/qla4xxx/ql4_nx.c
> +++ b/drivers/scsi/qla4xxx/ql4_nx.c
> @@ -406,6 +406,7 @@ void qla4_82xx_crb_win_unlock(struct scsi_qla_host *ha)
>  
>  void
>  qla4_82xx_wr_32(struct scsi_qla_host *ha, ulong off, u32 data)
> +	__context_unsafe(conditional locking)
>  {
>  	unsigned long flags = 0;
>  	int rv;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the locking
in this function, I noticed that the return value of qla4_82xx_crb_win_lock()
is completely ignored:

	write_lock_irqsave(&ha->hw_lock, flags);
	qla4_82xx_crb_win_lock(ha);
	qla4_82xx_pci_set_crbwindow_2M(ha, &off);

If another PCI function holds the hardware semaphore and the lock request
times out, qla4_82xx_crb_win_lock() returns -1. Does unconditionally
proceeding to reprogram the shared hardware CRB window defeat the mutual
exclusion mechanism and lead to uncoordinated window switching and hardware
state corruption?

[Severity: High]
This is also a pre-existing issue on this same path, but is there a risk
of a hard lockup panic here?

The write_lock_irqsave() call disables local interrupts just prior to entering
qla4_82xx_crb_win_lock(). Inside the lock acquisition routine, a spinloop
bounded by CRB_WIN_LOCK_TIMEOUT (100,000,000 iterations) calls udelay(10):

drivers/scsi/qla4xxx/ql4_nx.c:qla4_82xx_crb_win_lock() {
	...
		if (timeout >= CRB_WIN_LOCK_TIMEOUT)
			return -1;

		timeout++;
		udelay(10);
	...
}

If the hardware is unresponsive or highly contended, this massive spin loop
could run with IRQs disabled for up to 1,000 seconds. Will this invariably
trigger the kernel's hard lockup detector (NMI watchdog) and crash the system?

> @@ -429,6 +430,7 @@ qla4_82xx_wr_32(struct scsi_qla_host *ha, ulong off, u32 data)
>  }
>  
>  uint32_t qla4_82xx_rd_32(struct scsi_qla_host *ha, ulong off)
> +	__context_unsafe(conditional locking)
>  {
>  	unsigned long flags = 0;
>  	int rv;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the locking
primitives in this file, I noticed other places where hardware lock timeouts
are ignored.

For example, in qla4_82xx_pinit_from_rom():

	/* Halt all the indiviual PEGs and other blocks of the ISP */
	qla4_82xx_rom_lock(ha);

	/* disable all I2Q */
	qla4_82xx_wr_32(ha, QLA82XX_CRB_I2Q + 0x10, 0x0);

And similarly in qla4_82xx_isp_reset():

	qla4_82xx_idc_lock(ha);
	dev_state = qla4_82xx_rd_32(ha, QLA82XX_CRB_DEV_STATE);

If the lock is legitimately held by another host or PCI function and times
out, does blindly proceeding to execute hardware halt and reset operations
risk severe hardware state corruption?

[Severity: High]
This is another pre-existing issue in the same file's lock handling, but
is there a TOCTOU race condition when the hardware lock is temporarily dropped?

In qla4_82xx_pci_mem_read_direct() and qla4_82xx_pci_mem_write_direct(),
the protective hardware lock is dropped before performing I/O:

	addr = qla4_8xxx_pci_base_offsetfset(ha, start);
	if (!addr) {
		write_unlock_irqrestore(&ha->hw_lock, flags);
		mem_base = pci_resource_start(ha->pdev, 0);

While the lock is dropped, could another thread concurrently acquire
ha->hw_lock and reprogram the hardware window to a completely different
address? When the original thread re-acquires the lock and proceeds to
execute the I/O using the PCI pointer, will the hardware device forward
the access to the newly modified window, resulting in corrupted read/writes?

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