Re: [PATCH] usb: uas: implement .change_queue_depth to allow per-device queue depth override

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] uas: negative queue depth underflow via uas_change_queue_depth()
--

commit 9d52d5cb6887ffe601d9599b2dbfb63939e5e572
Author: Luca Cecchi <luca.cecchi.info@gmail.com>

usb: uas: implement .change_queue_depth to allow per-device queue depth override

This commit implements .change_queue_depth for UAS devices to allow
per-device queue depth overrides. This helps affected users lower the
depth for just their device without disabling UAS altogether.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..55291b4d0b825 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c

[ ... ]

> @@ -910,6 +910,27 @@ static int uas_sdev_configure(struct scsi_device *sdev,
>  	return 0;
>  }
>  
> +/*
> + * uas does not implement .change_queue_depth, so the standard sysfs
> + * queue_depth attribute stays read-only for every UAS device (see
> + * scsi_sysfs.c:sdev_store_queue_depth(), which requires
> + * sht->change_queue_depth != NULL). Some bridge chips become
> + * unstable under deep command queueing; wiring this up lets affected
> + * users lower the depth for just their device (e.g. via a udev rule
> + * matching idVendor/idProduct), without disabling UAS altogether.
> + * Default behaviour (qdepth - 2) is unchanged unless userspace asks
> + * for less.
> + */
> +static int uas_change_queue_depth(struct scsi_device *sdev, int depth)
> +{
> +	struct uas_dev_info *devinfo = (struct uas_dev_info *)sdev->host->hostdata;
> +	int max_depth = devinfo->qdepth - 2;
> +
> +	if (depth > max_depth)
> +		depth = max_depth;
> +	return scsi_change_queue_depth(sdev, depth);
> +}

[Severity: High]
Can this underflow queue_depth in the block layer if stream allocation
fails during a reset?

If a USB reset fails (e.g., via uas_post_reset() or uas_reset_resume()),
uas_configure_endpoints() can overwrite devinfo->qdepth with a negative
error code:

drivers/usb/storage/uas.c:uas_configure_endpoints() {
    ...
		devinfo->qdepth = usb_alloc_streams(devinfo->intf, eps + 1,
						    3, MAX_CMNDS, GFP_NOIO);
		if (devinfo->qdepth < 0)
			return devinfo->qdepth;
    ...
}

If devinfo->qdepth is negative (e.g. -ENOMEM), uas_change_queue_depth()
will calculate a negative max_depth and clamp the requested depth to this
negative limit.

When this negative depth is passed to scsi_change_queue_depth(), it
bypasses the depth > 0 condition but is still passed to
blk_set_queue_depth():

drivers/scsi/scsi.c:scsi_change_queue_depth() {
    ...
	if (depth > 0) {
		sdev->queue_depth = depth;
		wmb();
	}

	if (sdev->request_queue)
		blk_set_queue_depth(sdev->request_queue, depth);
    ...
}

Because blk_set_queue_depth() expects an unsigned int, the negative
value can underflow to a massive integer, corrupting q->queue_depth and
potentially breaking block layer QoS mechanisms (like WBT or Kyber).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008112645.1820678-1-luca.cecchi.info@gmail.com?part=1