Re: [PATCH 15/21] scsi: mpi3mr: Fix block_io accounting when a target is re-created during BSG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: mpi3mr: leaked block_io count in mpi3mr_tgtdev_block_io_dec()
- [Low] scsi: mpi3mr: undocumented seq member in struct mpi3mr_stgt_priv_data
--

commit 23e409e23e8d123d95d4c65a026b00b68a047101
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

scsi: mpi3mr: Fix block_io accounting when a target is re-created during BSG

This commit introduces a 64-bit sequence number on struct
mpi3mr_stgt_priv_data so that mpi3mr_tgtdev_block_io_dec() only decrements
block_io if the target private data matches the instance that was
incremented by mpi3mr_tgtdev_block_io_inc() when the BSG task management
command started.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> index da47100d895e7..8738fc2ce7e77 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr.h
> +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> @@ -847,6 +847,7 @@ struct mpi3mr_stgt_priv_data {
>  	u16 perst_id;
>  	u32 num_luns;
>  	atomic_t block_io;
> +	u64 seq;

[Severity: Low]
This isn't a bug, but struct mpi3mr_stgt_priv_data has a kernel-doc comment
block describing all of its members. Should a @seq entry be added there as
well so W=1 builds and scripts/kernel-doc do not warn about an undocumented
struct member?

[ ... ]

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index e7833b24c5097..67d41e4a0a0ab 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -788,6 +788,8 @@ struct mpi3mr_tgt_dev *mpi3mr_get_tgtdev_by_handle(
>  	return tgtdev;
>  }
>  
> +static atomic64_t mpi3mr_stgt_seq = ATOMIC64_INIT(0);
> +
>  /**
>   * mpi3mr_tgtdev_block_io_inc - Block I/O on the SCSI target of a tgtdev
>   * @mrioc: Adapter instance reference
> @@ -797,46 +799,52 @@ struct mpi3mr_tgt_dev *mpi3mr_get_tgtdev_by_handle(
>   * increment are done under tgtdev_lock, which mpi3mr_target_destroy holds
>   * while it frees the private data.
>   *
> - * Return: The private data that was incremented, only to be handed back to
> - * mpi3mr_tgtdev_block_io_dec. NULL if the tgtdev has no SCSI target.
> + * Return: Sequence number of the private data that was incremented, to be
> + * handed back to mpi3mr_tgtdev_block_io_dec. 0 if the tgtdev has no SCSI
> + * target.
>   */
> -struct mpi3mr_stgt_priv_data *mpi3mr_tgtdev_block_io_inc(struct mpi3mr_ioc *mrioc,
> -							 struct mpi3mr_tgt_dev *tgtdev)
> +u64 mpi3mr_tgtdev_block_io_inc(struct mpi3mr_ioc *mrioc,
> +			       struct mpi3mr_tgt_dev *tgtdev)
>  {
> -	struct mpi3mr_stgt_priv_data *stgt_priv = NULL;
> +	struct mpi3mr_stgt_priv_data *stgt_priv;
>  	unsigned long flags;
> +	u64 seq = 0;
>  
>  	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
>  	if (tgtdev->starget && tgtdev->starget->hostdata) {
>  		stgt_priv = tgtdev->starget->hostdata;
>  		atomic_inc(&stgt_priv->block_io);
> +		seq = stgt_priv->seq;
>  	}
>  	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> -	return stgt_priv;
> +	return seq;
>  }
>  
>  /**
>   * mpi3mr_tgtdev_block_io_dec - Undo mpi3mr_tgtdev_block_io_inc
>   * @mrioc: Adapter instance reference
>   * @tgtdev: Target device reference, held by the caller
> - * @stgt_priv: Value returned by mpi3mr_tgtdev_block_io_inc
> + * @seq: Value returned by mpi3mr_tgtdev_block_io_inc
>   *
> - * Decrement block_io only if the SCSI target still has the same private
> - * data. If the target was removed meanwhile the private data is gone
> - * and there is nothing to undo.
> + * Decrement block_io only if the SCSI target still has the private data
> + * that was incremented. A private data allocated later at the same address
> + * has a different sequence number and is left alone.
>   */
>  void mpi3mr_tgtdev_block_io_dec(struct mpi3mr_ioc *mrioc,
> -				struct mpi3mr_tgt_dev *tgtdev,
> -				struct mpi3mr_stgt_priv_data *stgt_priv)
> +				struct mpi3mr_tgt_dev *tgtdev, u64 seq)
>  {
> +	struct mpi3mr_stgt_priv_data *stgt_priv;
>  	unsigned long flags;
>  
> -	if (!stgt_priv)
> +	if (!seq)
>  		return;
>  
>  	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> -	if (tgtdev->starget && tgtdev->starget->hostdata == stgt_priv)
> -		atomic_dec_if_positive(&stgt_priv->block_io);
> +	if (tgtdev->starget && tgtdev->starget->hostdata) {
> +		stgt_priv = tgtdev->starget->hostdata;
> +		if (stgt_priv->seq == seq)
> +			atomic_dec_if_positive(&stgt_priv->block_io);
> +	}
>  	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
>  }

[Severity: Medium]
Can stgt_priv->block_io remain permanently elevated on a live scsi_target if
a BSG task management request completes while scsi_tgt_priv_data->num_luns
is temporarily zero?

In mpi3mr_sdev_destroy(), tgt_dev->starget is cleared whenever num_luns
reaches zero, even though the scsi_target and its hostdata (stgt_priv)
remain alive while starget->reap_ref is held:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sdev_destroy() {
	...
	scsi_tgt_priv_data->num_luns--;
	...
	if (tgt_dev && (!scsi_tgt_priv_data->num_luns))
		tgt_dev->starget = NULL;
	...
}

And when another LUN is initialized on that same scsi_target,
mpi3mr_sdev_init() sets tgt_dev->starget back to starget:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sdev_init() {
	...
	if (tgt_dev) {
		if (tgt_dev->starget == NULL)
			tgt_dev->starget = starget;
	...
}

This happens during __scsi_scan_target() (which holds starget->reap_ref)
when probing LUN 0 in scsi_probe_and_add_lun() returns
SCSI_SCAN_TARGET_PRESENT (such as when LUN 0 has peripheral qualifier 3) and
calls __scsi_remove_device() -> mpi3mr_sdev_destroy() before
scsi_report_lun_scan() or scsi_sequential_lun_scan() adds the remaining LUNs
on the same starget. It can also happen if the last LUN on a target is
removed via scsi_remove_device() while a concurrent __scsi_add_device() has
already acquired starget->reap_ref via scsi_alloc_target() and is waiting on
shost->scan_mutex.

If mpi3mr_bsg_process_mpt_cmds() calls mpi3mr_tgtdev_block_io_inc() while
LUN 0 exists, and then calls mpi3mr_tgtdev_block_io_dec() during the window
where tgt_dev->starget is NULL, mpi3mr_tgtdev_block_io_dec() sees
tgtdev->starget == NULL and skips the decrement. Once mpi3mr_sdev_init()
re-attaches tgt_dev->starget to that same starget, wouldn't
stgt_priv->block_io stay elevated and cause mpi3mr_qcmd() to return
SCSI_MLQUEUE_DEVICE_BUSY indefinitely for all commands to that target?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104120103.503871-1-ranjan.kumar@broadcom.com?part=15