Re: [PATCH v3 30/57] scsi: ibmvscsi_tgt: Enable lock context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incomplete implementation of `__must_hold` locking annotations.

Pre-existing issues:
- [High] Race condition during queue teardown leading to interrupt storm and `h_free_crq` collision.
--

--- Patch [30]: [PATCH v3 30/57] scsi: ibmvscsi_tgt: Enable lock context analysis ---
commit 3c00251f51d9e551b9d5359138425cfc65cd2cef
Author: Bart Van Assche <bvanassche@acm.org>

scsi: ibmvscsi_tgt: Enable lock context analysis

This commit enables lock context analysis for the ibmvscsi_tgt driver. It
documents locking requirements by adding the __must_hold() annotation to
several functions.

> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 61f6828007654..8e532a195bf95 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -251,6 +251,7 @@ static void ibmvscsis_delete_client_info(struct scsi_info *vscsi,
>   *	Process level, interrupt lock is held
>   */
>  static long ibmvscsis_free_command_q(struct scsi_info *vscsi)
> +	__must_hold(&vscsi->intr_lock)
>  {

[Severity: High]
This is a pre-existing issue, but could this sequence lead to an interrupt
storm or hypervisor faults due to concurrent CRQ access?

Looking at ibmvscsis_free_command_q(), it disables hardware interrupts and
then drops the lock:

    vio_disable_interrupts(vscsi->dma_dev);
    // ...
    spin_unlock_bh(&vscsi->intr_lock);
    rc = ibmvscsis_unregister_command_q(vscsi);
    spin_lock_bh(&vscsi->intr_lock);

If a pending ibmvscsis_handle_crq() tasklet on another CPU acquires the lock
while it is dropped, it evaluates TARGET_STOP(vscsi) as true and
unconditionally re-enables interrupts:

    if (TARGET_STOP(vscsi)) {
        vio_enable_interrupts(vscsi->dma_dev);
        // ...
        spin_unlock_bh(&vscsi->intr_lock);
        return;
    }

Would this cause ibmvscsis_unregister_command_q() to execute concurrently
with hardware interrupts actively firing?

[Severity: Low]
Are there other functions that explicitly require the interrupt lock but
were missed by this commit?

The commit message claims to "Document locking requirements with
__must_hold()", but several other functions in this file contain explicit
comments stating they must be called with the lock held, yet lack the
__must_hold() annotation.

For example, ibmvscsis_establish_new_q():

    /*
     * Must be called with interrupt lock held.
     */
    static long ibmvscsis_establish_new_q(struct scsi_info *vscsi)

And ibmvscsis_post_disconnect():

    /*
     * PRECONDITION:
     * interrupt lock is held
     */
    static void ibmvscsis_post_disconnect(struct scsi_info *vscsi,
                                          uint new_state, uint flag_bits)

Could these also be updated to ensure complete static analysis coverage?

>  	int bytes;
>  	u32 flags_under_lock;

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