Re: [PATCH] ufs: core: Disable queue depth tracking support

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

New issues:
- [High] Silently disabling userspace's ability to modify the UFS device queue depth via sysfs.
- [Medium] Unrelated and undocumented modification to the InfiniBand SRP target driver (`ib_srp.c`).

Pre-existing issues:
- [High] Unresolved concurrency and IRQ-handling bug in SCSI core (`scsi_track_queue_full`) and Block layer (`ioc_rqos_queue_depth_changed`).
--

--- Patch [1]: [PATCH] ufs: core: Disable queue depth tracking support ---
commit f181b5b33f08308dbca6a708c0dbc602ecd352b0
Author: Bart Van Assche <[email protected]>

ufs: core: Disable queue depth tracking support

Disable queue depth tracking support because UFS devices shouldn't need
queue tracking support. The queue depth tracking code is not thread-safe,
and scsi_track_queue_full() is called from interrupt context but not all
code called by it is IRQ-safe.

>      - scsi_track_queue_full() is called from interrupt context but not all
>        code called by scsi_track_queue_full() is IRQ-safe. Here is an
>        example of a call chain that may cause interrupts to be enabled,
>        something that is not allowed from interrupt context:
>     
>        ufshcd_mcq_poll_cqe_lock()
>          ufshcd_compl_one_cqe()
>            scsi_done()
>              blk_mq_complete_request()
>                scsi_complete()
>                  scsi_decide_disposition()
>                    scsi_handle_queue_full()
>                      scsi_track_queue_full()
>                        scsi_change_queue_depth()
>                          blk_set_queue_depth()
>                            rq_qos_queue_depth_changed()
>                              ioc_rqos_queue_depth_changed()
>                                spin_lock_irq()
>                                spin_unlock_irq() <- enables interrupts

[Severity: High]
This is a pre-existing issue, but by only disabling queue depth tracking
locally in the UFS driver, are dozens of other SCSI drivers that set
.track_queue_depth = 1 still exposed to this IRQ-safety bug?

Since ioc_rqos_queue_depth_changed() unconditionally uses spin_unlock_irq(),
and scsi_track_queue_full() has no locking for sdev->last_queue_full_count,
should this concurrency and IRQ unmasking issue be addressed in the SCSI core
and block layers directly?

> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 6b429ef63f8f3..d671a3d3359ba 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1044,6 +1044,10 @@ static void srp_remove_target(struct srp_target_port *target)
>  	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
>  
>  	srp_del_scsi_host_attr(target->scsi_host);
> +	/*
> +	 * The srp_remove_host() call decrements the rport reference count.
> +	 * Keep the rport as long as its needed.
> +	 */

[Severity: Medium]
Was this modification to the InfiniBand SRP target driver included by
accident?

The commit message only mentions disabling queue depth tracking for UFS, but
this introduces an undocumented change to srp_remove_target().

>  	srp_rport_get(target->rport);
>  	srp_remove_host(target->scsi_host);
>  	scsi_remove_host(target->scsi_host);

[ ... ]

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d3044a3089b53..bdb926a9f2d75 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -9580,7 +9566,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
>  	.sdev_init		= ufshcd_sdev_init,
>  	.sdev_configure		= ufshcd_sdev_configure,
>  	.sdev_destroy		= ufshcd_sdev_destroy,
> -	.change_queue_depth	= ufshcd_change_queue_depth,

[Severity: High]
Does removing .change_queue_depth from the template completely disable
userspace's ability to modify the queue depth via sysfs?

Looking at sdev_store_queue_depth(), it returns -EINVAL if this callback is
missing:

drivers/scsi/scsi_sysfs.c:sdev_store_queue_depth() {
    ...
	if (!sht->change_queue_depth)
		return -EINVAL;
    ...
}

Could .change_queue_depth be set to scsi_change_queue_depth instead to
preserve the manual sysfs tuning interface?

>  	.eh_abort_handler	= ufshcd_abort,
>  	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>  	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,

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