Re: [PATCH v3 38/57] scsi: lpfc: Prepare for enabling 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] An AB-BA lock inversion exists between `lpfc_cmd->buf_lock` and `phba->hbalock`, which can cause a hard deadlock.
- [High] Race condition in timer callback `lpfc_els_timeout_handler` leading to use-after-free on module unload.
- [High] List corruption due to missing lock protection for SLI ring lists in `lpfc_els_abort` and `lpfc_els_flush_cmd`.
- [High] Concurrent FCP abort requests will hang for the full timeout duration due to unshared wait queues.
--

--- Patch [38]: [PATCH v3 38/57] scsi: lpfc: Prepare for enabling lock context analysis ---
commit 7e811dc5ed08a4e45843970fc90d7859310944e2
Author: Bart Van Assche <bvanassche@acm.org>

scsi: lpfc: Prepare for enabling lock context analysis

Annotate the functions that perform conditional locking with
__context_unsafe().

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 52fc5058976d9..45e7f147b1636 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -9597,6 +9597,7 @@ lpfc_els_timeout(struct timer_list *t)
>   **/
>  void
>  lpfc_els_timeout_handler(struct lpfc_vport *vport)
> +	__context_unsafe(conditional locking)
>  {
>  	struct lpfc_hba  *phba = vport->phba;

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
here during module unload? 

Looking at the bottom of lpfc_els_timeout_handler(), the code checks for 
FC_UNLOADING before calling mod_timer():

    if (!test_bit(FC_UNLOADING, &phba->pport->load_flag))
        mod_timer(&vport->els_tmofunc, ...);

If the teardown thread sets the FC_UNLOADING flag immediately after this
check is evaluated, the callback will re-arm the timer. Even if
timer_delete_sync() is used during teardown, the callback re-arms the timer
before exiting, which can cause the timer to fire on freed memory and lead
to a use-after-free crash.

> @@ -9719,6 +9720,7 @@ lpfc_els_timeout_handler(struct lpfc_vport *vport)
>   **/
>  void
>  lpfc_els_flush_cmd(struct lpfc_vport *vport)
> +	__context_unsafe(conditional locking)
>  {
>  	LIST_HEAD(abort_list);

[Severity: High]
This is a pre-existing issue, but can list corruption occur here during ELS
command flushes? 

In lpfc_els_flush_cmd(), this code moves piocb->list from txcmplq to
cancel_list:

    spin_lock_irqsave(&phba->hbalock, iflags);
    list_del_init(&piocb->dlist);
    if (mbx_tmo_err || !(phba->sli.sli_flag & LPFC_SLI_ACTIVE))
        list_move_tail(&piocb->list, &cancel_list);

For SLI4, is holding only hbalock sufficient, or does it also require 
pring->ring_lock to safely modify the txcmplq, preventing kernel panics 
during linked list traversal?

> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> index 9c449055a55e6..79e5e876c879e 100644
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
> @@ -220,6 +220,7 @@ lpfc_check_elscmpl_iocb(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>   */
>  void
>  lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
> +	__context_unsafe(conditional locking)
>  {
>  	LIST_HEAD(abort_list);

[Severity: High]
This isn't a bug introduced by this patch, but does this list modification
lack lock protection? 

In lpfc_els_abort(), list_del_init() modifies the active txcmplq without
holding any locks:

    if (retval && test_bit(FC_UNLOADING, &phba->pport->load_flag)) {
        list_del_init(&iocb->list);
        list_add_tail(&iocb->list, &drv_cmpl_list);
    }

Could this lead to list corruption during error recovery scenarios?

> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index f2cab134af7f1..6c3fd3c33c535 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -5520,6 +5520,7 @@ void lpfc_vmid_vport_cleanup(struct lpfc_vport *vport)
>   **/
>  static int
>  lpfc_abort_handler(struct scsi_cmnd *cmnd)
> +	__context_unsafe(conditional locking)
>  {
>  	struct Scsi_Host  *shost = cmnd->device->host;

[Severity: High]
This is a pre-existing issue, but does this locking order create an AB-BA
deadlock? 

In lpfc_abort_handler(), lpfc_cmd->buf_lock is acquired followed by 
phba->hbalock:

    spin_lock_irqsave(&lpfc_cmd->buf_lock, flags);
    spin_lock(&phba->hbalock);

In lpfc_sli_abort_taskmgmt(), the thread acquires phba->hbalock first, 
iterates over commands, and attempts to acquire lpfc_cmd->buf_lock. Could 
concurrent SCSI aborts and device resets freeze storage operations?

[Severity: High]
This is also a pre-existing issue, but can concurrent FCP abort requests
hang here?

In lpfc_abort_handler(), if a second thread attempts to abort the same 
command, it detects LPFC_DRIVER_ABORTED and jumps to wait_for_cmpl:

wait_for_cmpl:
    wait_event_timeout(waitq, (lpfc_cmd->pCmd != cmnd),
                       secs_to_jiffies(2*vport->cfg_devloss_tmo));

It then sleeps on its private, on-stack waitq. Because the completion 
handler only knows about the first thread's wait queue, won't the second 
thread block unconditionally for the full duration of the timeout?

> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 62a30a92b7927..c76d62e8907c1 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c

[ ... ]

> @@ -12871,6 +12872,7 @@ lpfc_sli_abort_iocb(struct lpfc_vport *vport, u16 tgt_id, u64 lun_id,
>  int
>  lpfc_sli_abort_taskmgmt(struct lpfc_vport *vport, struct lpfc_sli_ring *pring,
>  			uint16_t tgt_id, uint64_t lun_id, lpfc_ctx_cmd cmd)
> +	__context_unsafe(conditional locking)
>  {
>  	struct lpfc_hba *phba = vport->phba;

[Severity: High]
This isn't a bug introduced by this patch, but as mentioned above, does this
lock ordering conflict with lpfc_abort_handler()? 

In lpfc_sli_abort_taskmgmt(), phba->hbalock is acquired before
lpfc_cmd->buf_lock:

    spin_lock_irqsave(&phba->hbalock, iflags);
    ...
    lpfc_cmd = container_of(iocbq, struct lpfc_io_buf, cur_iocbq);
    spin_lock(&lpfc_cmd->buf_lock);

This is the reverse of the order in lpfc_abort_handler(). Can this lead to 
a hard deadlock during concurrent I/O timeouts?

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