Re: [PATCH v3 08/57] scsi: aacraid: Prepare for enabling lock context analysis

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

Pre-existing issues:
- [Critical] Data race on `fibptr->flags` in `aac_hba_send` and `aac_fib_send` can lead to lost flags set by the interrupt handler.
- [High] Permanent leak of `dev->management_fib_count` on early error paths in `aac_fib_send` and `aac_hba_send`.
- [High] Dangling pointer in `dev->sync_fib_list` when `wait_for_completion_interruptible` is interrupted by a signal.
- [High] The `_aac_reset_adapter()` function can sleep while holding a spinlock (`host->host_lock`) and with interrupts disabled.
--

--- Patch [8]: [PATCH v3 08/57] scsi: aacraid: Prepare for enabling lock context analysis ---
commit b24c9703822591fc27fa76398a50df3f082577ac
Author: Bart Van Assche <[email protected]>

scsi: aacraid: Prepare for enabling lock context analysis

Document the aac_send_reset_adapter() locking requirements with
__must_hold(). Annotate functions that perform conditional locking with
__no_context_analysis.

> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index c4485629f7923..fb4d78233c6d7 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -475,6 +475,7 @@ int aac_queue_get(struct aac_dev * dev, u32 * index, u32 qid, struct hw_fib * hw
>  int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
>  		int priority, int wait, int reply, fib_callback callback,
>  		void *callback_data)
> +	__context_unsafe(conditional locking)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does an interrupted wait leave a
dangling pointer in dev->sync_fib_list?

In aac_fib_send(), if dev->sync_mode is enabled and wait is non-zero, fibptr
is appended to dev->sync_fib_list. The code then waits:

    if (wait_for_completion_interruptible(&fibptr->event_wait)) {
        fibptr->flags &= ~FIB_CONTEXT_FLAG_WAIT;
        return -EFAULT;
    }

If interrupted by a signal, it returns -EFAULT without removing fibptr from
dev->sync_fib_list. The caller will free the FIB, adding it to the free list
while it remains on sync_fib_list. Will this lead to a use-after-free when
dev->sync_fib_list is later processed?

[ ... ]

> @@ -698,6 +699,7 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
>  
>  int aac_hba_send(u8 command, struct fib *fibptr, fib_callback callback,
>  		void *callback_data)
> +	__context_unsafe(conditional locking)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that
dev->management_fib_count leaks on early error paths?

In aac_hba_send(), when wait is 1, dev->management_fib_count is incremented.
If aac_adapter_deliver() succeeds, but the PCI device goes offline:

    if (wait) {
        spin_unlock_irqrestore(&fibptr->event_lock, flags);

        if (unlikely(aac_pci_offline(dev)))
            return -EFAULT;

The function returns -EFAULT directly without decrementing the management count.
Because fibptr->done is not set to 2, the interrupt handler will also not
decrement it. A similar leak appears to occur in aac_fib_send().

Will this eventually exhaust the AAC_NUM_MGT_FIB pool and cause future
management commands to fail with -EBUSY?

[Severity: Critical]
This isn't a bug introduced by this patch, but does the lockless update of
fibptr->flags risk losing flags set by the interrupt handler?

In aac_hba_send(), after the command is submitted via aac_adapter_deliver():

    if (wait) {
        ...
        fibptr->flags |= FIB_CONTEXT_FLAG_WAIT;
        if (wait_for_completion_interruptible(&fibptr->event_wait))
            fibptr->done = 2;
        fibptr->flags &= ~(FIB_CONTEXT_FLAG_WAIT);

These bitwise operations are not atomic. Concurrently, the adapter can
complete the command and trigger the interrupt handler aac_intr_normal(),
which locklessly sets flags:

    if (isFastResponse)
        fib->flags |= FIB_CONTEXT_FLAG_FASTRESP;

Can the read-modify-write in aac_hba_send() overwrite and lose the
FIB_CONTEXT_FLAG_FASTRESP flag, causing the completion path to misinterpret
the response?

[ ... ]

> @@ -1466,6 +1468,7 @@ static void aac_schedule_bus_scan(struct aac_dev *aac)
>  }
>  
>  static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
> +	__context_unsafe(conditional locking)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does _aac_reset_adapter() sleep
while holding a spinlock?

aac_reset_adapter() acquires host->host_lock (a spinlock) and calls
_aac_reset_adapter(). Inside _aac_reset_adapter(), the lock is conditionally
dropped (setting jafo = 1) only if aac->thread is not NULL and the current
thread is not the command thread.

If aac->thread is NULL, or if called by the command thread itself, jafo
remains 0 and the spinlock is never unlocked. The function then proceeds to
call blocking functions like schedule() and ssleep():

    if (is_response_expected || fib->flags & FIB_CONTEXT_FLAG_WAIT) {
        ...
        schedule();
        retval = 0;
    }

    /* Give some extra time for ioctls to complete. */
    if (retval == 0)
        ssleep(2);

Could this lead to a deadlock since the spinlock is held with interrupts
disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8