Re: [PATCH v3 52/57] scsi: smartpqi: Enable lock context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch introduces fake `__acquire()` directives to artificially balance the static analyzer's lock count, and the commit message incorrectly claims to use `__must_hold()` and `__no_context_analysis()` which are entirely absent.

Pre-existing issues:
- [High] The driver violates strict `struct mutex` ownership rules by acquiring mutexes in one execution context and releasing them in another, resulting in cross-task unlock violations.
- [High] Error paths in `pqi_resume_or_restore` return early without releasing `scan_mutex` and `lun_reset_mutex`, causing a permanent lock leak.
- [High] Blindly unlocking `ofa_mutex` during an OFA cancellation event in `pqi_ofa_process_event()` without verifying it is held causes undefined behavior.
--

--- Patch [52]: [PATCH v3 52/57] scsi: smartpqi: Enable lock context analysis ---
Please note that due to recitation restrictions, this review uses a free-form
summary format rather than quoting the original patch diff directly.

commit 8564515b7a0cc93357264da233ee9448ed603eee
Author: Bart Van Assche <bvanassche@acm.org>
scsi: smartpqi: Enable lock context analysis

This patch aims to document locking requirements in the smartpqi driver using
sparse annotations like __acquires() and __releases() to enable lock context
analysis.

[Severity: Medium]
The commit message states that this patch annotates functions with
__must_hold() and __no_context_analysis, but neither of those seem to be
used in the actual code.

Additionally, in functions like pqi_process_soft_reset() and
pqi_update_device_list(), the patch adds fake __acquire() directives with
comments asking what guarantees the mutexes are held. Does this misuse
the sparse annotations by artificially balancing the lock count just to
suppress warnings, instead of fixing or properly documenting the locking
design?

[Severity: High]
This isn't a bug introduced by this patch, but the driver appears to
violate strict struct mutex ownership rules. For example,
pqi_ofa_memory_alloc_worker() acquires the ofa_mutex, but the lock is later
released by a completely different worker thread in pqi_ofa_quiesce_worker()
(via pqi_process_soft_reset()). 

Similarly, pqi_suspend_or_freeze() locks scan_mutex and lun_reset_mutex
prior to sleep, which are then released by resume operations, potentially
across different async PM threads.

Since struct mutex requires the locking task to be the one that unlocks it,
could this lead to DEBUG_LOCKS_WARN_ON, workqueue lock leak warnings, and
potential internal mutex state corruption during firmware events or system PM
operations?

[Severity: High]
This is a pre-existing issue, but in pqi_resume_or_restore(), if the driver
encounters a disconnected device (ctrl_removal_state != PQI_CTRL_PRESENT) or
if pqi_resume_ctrl() fails, the function returns early. 

Since it returns without calling pqi_ctrl_unblock_device_reset() and
pqi_ctrl_unblock_scan(), does this cause a permanent resource leak of
scan_mutex and lun_reset_mutex? This could leave the driver deadlocked for
any future operations requiring these locks. The patch seems to legitimize
this leak to sparse by adding __cond_releases() annotations.

[Severity: High]
This is a pre-existing issue, but in pqi_ofa_process_event(), when handling
the PQI_EVENT_OFA_CANCELED event, the code calls pqi_ctrl_ofa_done() which
blindly unlocks ofa_mutex.

If a cancellation event is received from the controller firmware when no OFA
memory allocation is in progress, or before the worker thread has had a
chance to acquire the mutex, wouldn't unlocking an unheld mutex trigger a
DEBUG_LOCKS_WARN_ON and corrupt the mutex state?

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