Re: [PATCH v3 28/57] scsi: hpsa: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `hpsa_send_reset` fails with a SCSI error after being submitted to the hardware, `dev->commands_outstanding` is leaked.
- [High] Concurrent device reset requests cause premature clearing of mutual exclusion flags (`reset_in_progress` and `in_reset`), leaving the controller vulnerable to rescans and new OS commands while a reset is actively in progress.
--

--- Patch [28]: [PATCH v3 28/57] scsi: hpsa: Prepare for enabling lock context analysis ---
commit 9b4522a3e2cfce6e65e269b286bd08359ef94086
Author: Bart Van Assche <bvanassche@acm.org>

scsi: hpsa: Prepare for enabling lock context analysis

mutex_lock_interruptible() returns a negative value upon failure or zero
upon success. Since the Clang thread-safety analyzer only supports == 0
and != 0 tests for functions that perform conditional locking, change
the == -EINTR test into != 0. This change does not modify the behavior
of hpsa_do_reset().

> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 8edad1830abe4..3f38030d6a8d1 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3163,7 +3163,7 @@ static int hpsa_do_reset(struct ctlr_info *h, struct hpsa_scsi_dev_t *dev,
>  	int rc = 0;
>  
>  	/* We can really only handle one reset at a time */
> -	if (mutex_lock_interruptible(&h->reset_mutex) == -EINTR) {
> +	if (mutex_lock_interruptible(&h->reset_mutex)) {

[Severity: High]
This is a pre-existing issue, but concurrent device reset requests cause
premature clearing of mutual exclusion flags (reset_in_progress and in_reset),
leaving the controller vulnerable to rescans and new OS commands while a reset
is actively in progress.

If Thread 1 is performing a reset and Thread 2 is blocked on
mutex_lock_interruptible(&h->reset_mutex), interrupting Thread 2 (e.g., via
Ctrl-C) causes it to exit hpsa_do_reset() and unconditionally clear
h->reset_in_progress and dev->in_reset in hpsa_eh_device_reset_handler():

drivers/scsi/hpsa.c:hpsa_eh_device_reset_handler() {
    ...
return_reset_status:
	spin_lock_irqsave(&h->reset_lock, flags);
	h->reset_in_progress = 0;
	if (dev)
		dev->in_reset = false;
	spin_unlock_irqrestore(&h->reset_lock, flags);
}

Does this strip the mutual exclusion protection from Thread 1, exposing the
hardware to concurrent commands?

[Severity: High]
This isn't a bug introduced by this patch, but if hpsa_send_reset() fails with
a SCSI error after being submitted to the hardware, does it leak
dev->commands_outstanding?

If hpsa_send_reset() successfully enqueues the command (incrementing the
counter) but the hardware later fails the command (e.g., CMD_TARGET_STATUS),
hpsa_send_reset() returns a non-zero error code. 

hpsa_do_reset() sees this non-zero rc and skips
atomic_dec(&dev->commands_outstanding):

drivers/scsi/hpsa.c:hpsa_do_reset() {
    ...
	rc = hpsa_send_reset(h, dev, reset_type, reply_queue);
	if (!rc) {
		/* incremented by sending the reset request */
		atomic_dec(&dev->commands_outstanding);
    ...
}

Because CMD_IOCTL_PEND commands do not decrement this counter in their
interrupt completion handler, finish_cmd():

drivers/scsi/hpsa.c:finish_cmd() {
    ...
	else if (c->cmd_type == CMD_IOCTL_PEND || c->cmd_type == IOACCEL2_TMF)
		complete(c->waiting);
    ...
}

Is the count permanently leaked, causing an infinite hang on the next reset
attempt for the device because wait_event() will block forever?

>  		dev_warn(&h->pdev->dev, "concurrent reset wait interrupted.\n");
>  		return -EINTR;
>  	}

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