Re: [PATCH v3 42/57] scsi: mvsas: 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:
- [Critical] Dropping the spinlock inside a `list_for_each_entry_safe` loop in `mvs_do_release_task` allows list modification, causing an infinite loop.
- [Critical] Dropping the spinlock inside `mvs_int_rx` allows concurrent execution, causing double-completion of tasks and potential Use-After-Free.
- [High] The function `mvs_port_notify_deformed` fails to acquire `mvi->lock` and was missed by the new locking annotations, causing a lock imbalance.
- [High] Using a global PHY index instead of a local PHY index to write to hardware registers in `mvs_port_notify_formed`.
--

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

scsi: mvsas: Enable lock context analysis

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

> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 359226e80eae8..f63e0dc1abd23 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -1055,6 +1055,7 @@ void mvs_update_phyinfo(struct mvs_info *mvi, int i, int get_st)
>  }
>  
>  static void mvs_port_notify_formed(struct asd_sas_phy *sas_phy, int lock)
> +	__context_unsafe(conditional locking)
>  {
>  	struct sas_ha_struct *sas_ha = sas_phy->ha;
>  	struct mvs_info *mvi = NULL; int i = 0, hi;

[Severity: High]
This is a pre-existing issue, but does mvs_port_notify_formed() write to the
wrong hardware offset when multiple HBAs are present?

It uses the global index i instead of the local PHY index sas_phy->id:

drivers/scsi/mvsas/mv_sas.c:mvs_port_notify_formed() {
    ...
    if (phy->att_dev_info & PORT_SSP_TRGT_MASK) {
        MVS_CHIP_DISP->write_port_cfg_addr(mvi, i, PHYR_PHY_STAT);
        MVS_CHIP_DISP->write_port_cfg_data(mvi, i, 0x04);
    }
    ...
}

When the global index is greater than or equal to mvi->chip->n_phy, could
passing it to write_port_cfg_addr() cause out-of-bounds memory-mapped IO
writes?

[ ... ]

> @@ -1644,6 +1647,7 @@ int mvs_slot_complete(struct mvs_info *mvi, u32 rx_desc, u32 flags)
>  
>  void mvs_do_release_task(struct mvs_info *mvi,
>  		int phy_no, struct domain_device *dev)
> +	__must_hold(&mvi->lock)
>  {
>  	u32 slot_idx;
>  	struct mvs_phy *phy;

[Severity: Critical]
This is a pre-existing issue, but is it safe to use list_for_each_entry_safe()
here if the lock is dropped inside mvs_slot_complete()?

mvs_do_release_task() loops over the port list:

drivers/scsi/mvsas/mv_sas.c:mvs_do_release_task() {
    ...
    list_for_each_entry_safe(slot, slot2, &port->list, entry) {
        ...
        mvs_slot_complete(mvi, slot_idx, 1);
    }
    ...
}

When mvs_slot_complete() temporarily releases mvi->lock, a concurrent
interrupt could complete the cached next element (slot2) and remove it from
the list. 

Would this leave slot2->entry.next pointing to itself, causing
mvs_do_release_task() to spin forever in an infinite loop when it resumes?

[Severity: High]
Also a pre-existing issue, but with the new __must_hold annotation here, what
happens when this is called from mvs_port_notify_deformed()?

mvs_port_notify_deformed() ignores its lock parameter and directly calls
mvs_do_release_task() without holding mvi->lock:

drivers/scsi/mvsas/mv_sas.c:mvs_port_notify_deformed() {
    ...
    list_for_each_entry(dev, &port->dev_list, dev_list_node)
        mvs_do_release_task(phy->mvi, phy_no, dev);
    ...
}

Will this cause a lock imbalance when the downstream mvs_slot_complete()
call unconditionally executes spin_unlock(&mvi->lock)?

[ ... ]

> @@ -1862,6 +1868,7 @@ void mvs_int_port(struct mvs_info *mvi, int phy_no, u32 events)
>  }
>  
>  int mvs_int_rx(struct mvs_info *mvi, bool self_clear)
> +	__must_hold(&mvi->lock)
>  {
>  	u32 rx_prod_idx, rx_desc;
>  	bool attn = false;

[Severity: Critical]
This is a pre-existing issue, but does dropping the lock inside mvs_int_rx()
expose mvi->rx_cons to concurrent modification?

mvs_int_rx() loops while evaluating the shared mvi->rx_cons:

drivers/scsi/mvsas/mv_sas.c:mvs_int_rx() {
    ...
    while (mvi->rx_cons != rx_prod_idx) {
        ...
        if (likely(rx_desc & RXQ_DONE))
            mvs_slot_complete(mvi, rx_desc, 0);
        ...
    }
    ...
}

Inside the loop, mvs_slot_complete() drops mvi->lock. If a concurrent
interrupt on another CPU acquires the lock, processes the ring, and frees
the tasks, could the first CPU wake up, read old descriptors based on the
updated mvi->rx_cons, and erroneously complete new reused tasks?

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