[RFCv2]preliminary support for eh_abort_handler

[RFCv2]preliminary support for eh_abort_handler

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

Hi,

the older version had serious bugs.

	Regards
		Oliver

[-- Attachment #2: 0001-usb-uas-cancel-outstanding-commands.patch --]
[-- Type: text/x-patch, Size: 9669 bytes --]

From feee0bd0c5e010d99866d5511dcb0c1c01f89c37 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 4 Nov 2026 11:17:34 +0100
Subject: [PATCH] usb: uas: cancel outstanding commands

Third draft of an actually working eh_abort_handler.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/storage/uas.c | 198 ++++++++++++++++++++++++++++++++++----
 1 file changed, 181 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8655edbd66b1..7eddc6633c4d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -38,6 +38,7 @@ struct uas_dev_info {
 	struct usb_anchor cmd_urbs;
 	struct usb_anchor sense_urbs;
 	struct usb_anchor data_urbs;
+	struct usb_anchor killer_urbs;
 	u64 flags;
 	int qdepth, resetting;
 	unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -71,6 +72,9 @@ struct uas_cmd_info {
 	struct urb *cmd_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
+	struct urb *killer;
+	wait_queue_head_t cancel;
+	int cleanly_cancelled;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -166,7 +170,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
 		cmdinfo = scsi_cmd_priv(cmnd);
 		uas_log_cmd_state(cmnd, __func__, 0);
 		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
-		cmdinfo->state &= ~COMMAND_INFLIGHT;
+		cmdinfo->state &= ~(COMMAND_INFLIGHT | COMMAND_ABORTED);
 		cmnd->result = result << 16;
 		err = uas_try_complete(cmnd, __func__);
 		WARN_ON(err != 0);
@@ -286,6 +290,9 @@ static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *
 	case RC_TMF_NOT_SUPPORTED:
 		set_host_byte(cmnd, DID_BAD_TARGET);
 		break;
+	case RC_OVERLAPPED_TAG:
+		set_host_byte(cmnd, DID_ABORT);
+		break;
 	default:
 		uas_log_cmd_state(cmnd, "response iu", response_code);
 		set_host_byte(cmnd, DID_ERROR);
@@ -678,6 +685,7 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 	memset(cmdinfo, 0, sizeof(*cmdinfo));
 	cmdinfo->uas_tag = idx + 1; /* uas-tag == usb-stream-id, so 1 based */
 	cmdinfo->state = SUBMIT_STATUS_URB | ALLOC_CMD_URB | SUBMIT_CMD_URB;
+	init_waitqueue_head(&cmdinfo->cancel);
 
 	switch (cmnd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
@@ -730,35 +738,188 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 static DEF_SCSI_QCMD(uas_queuecommand)
 
 /*
- * For now we do not support actually sending an abort to the device, so
- * this eh always fails. Still we must define it to make sure that we've
- * dropped all references to the cmnd in question once this function exits.
+ * We abort a command by reusing the tag, thus forcing an error
  */
+
+static struct command_iu *allocate_double_command(int tag, struct scsi_device *sdev)
+{
+	struct command_iu *test_unit_ready;
+
+	test_unit_ready = kzalloc(sizeof(*test_unit_ready), GFP_ATOMIC);
+	if (!test_unit_ready)
+		return NULL;
+
+	test_unit_ready->iu_id = IU_ID_COMMAND;
+	test_unit_ready->tag = cpu_to_be16(tag);
+	test_unit_ready->prio_attr = UAS_SIMPLE_TAG;
+	int_to_scsilun(sdev->lun, &test_unit_ready->lun);
+	/*
+	 * We need something that does not do harm
+	 * if the command to be aborted does finish
+	 * regularly
+	 */
+	test_unit_ready->cdb[0] = TEST_UNIT_READY;
+
+	return test_unit_ready;
+}
+
+static void uas_dummy_sense(struct urb *u)
+{
+	struct scsi_cmnd *cmnd = u->context;
+	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
+	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+	int tag = cmdinfo->uas_tag;
+	unsigned long flags;
+	/*
+	 * An error implies success
+	 */
+	int status = u->status;
+
+	/*
+	 * The tag must be freed in every case
+	 */
+	spin_lock_irqsave(&devinfo->lock, flags);
+	devinfo->cmnd[tag - 1] = NULL;
+	switch (status) {
+	case -ESHUTDOWN:
+	case -ENOENT:
+	case -ECONNRESET:
+	case 0:
+		cmdinfo->cleanly_cancelled = 1;
+		break;
+	}
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
+	kfree(u->transfer_buffer);
+	wake_up_all(&cmdinfo->cancel);
+}
+
+static void uas_abort_complete(struct urb *u)
+{
+	int status = u->status;
+	struct scsi_cmnd *cmnd = u->context;
+	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
+	struct usb_device *udev = devinfo->udev;
+	struct response_iu *dummy;
+	int tag = cmdinfo->uas_tag;
+	unsigned long flags;
+	int rv;
+
+	switch (status) {
+	default:
+		/*
+		 * immediate cleanup
+		 */
+		kfree(u->transfer_buffer);
+		spin_lock_irqsave(&devinfo->lock, flags);
+		devinfo->cmnd[tag - 1] = NULL;
+		spin_unlock_irqrestore(&devinfo->lock, flags);
+		wake_up_all(&cmdinfo->cancel);
+		break;
+	case 0:
+		kfree(u->transfer_buffer);
+		dummy = kzalloc(sizeof(*dummy), GFP_ATOMIC);
+		if (!dummy) {
+			spin_lock_irqsave(&devinfo->lock, flags);
+			goto bailout;
+		}
+		usb_fill_bulk_urb(cmdinfo->killer, udev, devinfo->status_pipe,
+				dummy, sizeof(*dummy), uas_dummy_sense, cmnd);
+		spin_lock_irqsave(&devinfo->lock, flags);
+		if (devinfo->use_streams)
+			cmdinfo->killer->stream_id = cmdinfo->uas_tag;
+		usb_anchor_urb(u, &devinfo->killer_urbs);
+		rv = usb_submit_urb(cmdinfo->killer, GFP_ATOMIC);
+		if (rv < 0) {
+bailout:
+			devinfo->cmnd[tag - 1] = NULL;
+			spin_unlock_irqrestore(&devinfo->lock, flags);
+			usb_unanchor_urb(u);
+			kfree(dummy);
+			usb_free_urb(u);
+		} else {
+			spin_unlock_irqrestore(&devinfo->lock, flags);
+		}
+		break;
+	}
+}
+
 static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 {
+	DECLARE_WAITQUEUE(wait, current);
 	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
 	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
+	struct usb_device *udev = devinfo->udev;
 	struct urb *data_in_urb = NULL;
 	struct urb *data_out_urb = NULL;
-	unsigned long flags;
+	struct urb *killer = NULL;
+	struct command_iu *bullet = NULL;
+	int target_tag, rv;
+	int result = FAILED;
 
-	spin_lock_irqsave(&devinfo->lock, flags);
+	killer = usb_alloc_urb(0, GFP_NOIO);
 
-	uas_log_cmd_state(cmnd, __func__, 0);
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	add_wait_queue(&cmdinfo->cancel, &wait);
+	spin_lock_irq(&devinfo->lock);
+	if (devinfo->resetting)
+		goto futile;
+	target_tag = cmdinfo->uas_tag;
 
 	/* Ensure that try_complete does not call scsi_done */
 	cmdinfo->state |= COMMAND_ABORTED;
 
-	/* Drop all refs to this cmnd, kill data urbs to break their ref */
-	devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
-	if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
-		data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
-	if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
-		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
-
-	uas_free_unsubmitted_urbs(cmnd);
+	if (!killer)
+		goto admit_failure;
+	cmdinfo->killer = killer;
+	bullet = allocate_double_command(target_tag, cmnd->device);
+	if (!bullet)
+		goto admit_failure;
+
+	usb_fill_bulk_urb(killer, udev, devinfo->cmd_pipe,
+			bullet, sizeof(*bullet), uas_abort_complete, cmnd);
+
+	cmdinfo->killer = killer;
+
+	usb_anchor_urb(killer, &devinfo->killer_urbs);
+	rv = usb_submit_urb(killer, GFP_ATOMIC);
+	if (rv < 0) {
+admit_failure:
+		/* Drop all refs to this cmnd, kill data urbs to break their ref */
+		devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
+		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
 
-	spin_unlock_irqrestore(&devinfo->lock, flags);
+		uas_free_unsubmitted_urbs(cmnd);
+		spin_unlock_irq(&devinfo->lock);
+		usb_unanchor_urb(killer);
+		kfree(bullet);
+	} else {
+		spin_unlock_irq(&devinfo->lock);
+		rv = schedule_timeout(HZ);
+		if (rv)
+			result = cmdinfo->cleanly_cancelled ? SUCCESS : FAILED;
+		spin_lock_irq(&devinfo->lock);
+		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+		spin_unlock_irq(&devinfo->lock);
+	}
+	set_current_state(TASK_RUNNING);
+	remove_wait_queue(&cmdinfo->cancel, &wait);
+	/*
+	 * we know a callback will free the tag
+	 */
+	spin_lock_irq(&devinfo->lock);
+	cmdinfo->killer = NULL;
+futile:
+	spin_unlock_irq(&devinfo->lock);
+	usb_kill_urb(killer);
+	usb_free_urb(killer);
 
 	if (data_in_urb) {
 		usb_kill_urb(data_in_urb);
@@ -769,7 +930,7 @@ static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 		usb_put_urb(data_out_urb);
 	}
 
-	return FAILED;
+	return result;
 }
 
 static int uas_eh_host_reset_handler(struct scsi_cmnd *cmnd)
@@ -796,6 +957,7 @@ static int uas_eh_host_reset_handler(struct scsi_cmnd *cmnd)
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
+	usb_kill_anchored_urbs(&devinfo->killer_urbs);
 	uas_zap_pending(devinfo, DID_RESET);
 
 	err = usb_reset_device(udev);
@@ -1038,6 +1200,7 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	init_usb_anchor(&devinfo->cmd_urbs);
 	init_usb_anchor(&devinfo->sense_urbs);
 	init_usb_anchor(&devinfo->data_urbs);
+	init_usb_anchor(&devinfo->killer_urbs);
 	spin_lock_init(&devinfo->lock);
 	INIT_WORK(&devinfo->work, uas_do_work);
 	INIT_WORK(&devinfo->scan_work, uas_scan_work);
@@ -1225,6 +1388,7 @@ static void uas_disconnect(struct usb_interface *intf)
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
 	usb_kill_anchored_urbs(&devinfo->data_urbs);
+	usb_kill_anchored_urbs(&devinfo->killer_urbs);
 	uas_zap_pending(devinfo, DID_NO_CONNECT);
 
 	/*
-- 
2.56.0

Re: [RFCv2]preliminary support for eh_abort_handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] uas: use-after-free and double-free of killer URB via uas_abort_complete()
- [Critical] uas: stack waitqueue corruption and task state leak in uas_eh_abort_handler()
- [High] uas: NULL dereference on cmdinfo->killer in uas_abort_complete()
- [High] uas: leak of unsubmitted URBs on normal abort path in uas_eh_abort_handler()
- [High] uas: duplicate status URB behind active sense URB in uas_abort_complete()
- [High] uas: uas_dummy_sense() treats URB unlink errors as clean command cancellation
- [High] uas: tag slot cleared in devinfo->cmnd[] before killing in-flight URBs
- [High] uas: uas_eh_abort_handler() does not verify cmnd still owns target_tag
- [Low] uas: missing subsystem prefix in subject and informal commit message
--

commit 3bd3a607b7fe69446419fd00011c11e8b7327c84
Author: Oliver Neukum <oneukum@suse.com>

preliminary support for eh_abort_handler

This commit implements command abort support in uas_eh_abort_handler() by
submitting a duplicate TEST_UNIT_READY command IU reusing the timed-out
command's tag to trigger an overlapped-tag error on the UAS target, followed
by a status URB to collect the response.

>     preliminary support for eh_abort_handler
>     
>     Hi,
>     
>     the older version had serious bugs.
>     
>             Regards
>                     Oliver
>     
>     Third draft of an actually working eh_abort_handler.
>     
>     Signed-off-by: Oliver Neukum <oneukum@suse.com>

[Severity: Low]
This isn't a bug, but the subject line is missing the uas: (or usb: storage:
uas:) subsystem prefix, and the commit message contains email salutations
rather than a technical description. Could the changelog explain why commands
are aborted by reusing the tag with a TEST_UNIT_READY command IU instead of
issuing a UAS task management function IU?

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

[ ... ]

> @@ -166,7 +170,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
>  		cmdinfo = scsi_cmd_priv(cmnd);
>  		uas_log_cmd_state(cmnd, __func__, 0);
>  		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
> -		cmdinfo->state &= ~COMMAND_INFLIGHT;
> +		cmdinfo->state &= ~(COMMAND_INFLIGHT | COMMAND_ABORTED);
>  		cmnd->result = result << 16;
>  		err = uas_try_complete(cmnd, __func__);
>  		WARN_ON(err != 0);

[Severity: High]
If uas_disconnect() or uas_eh_host_reset_handler() calls uas_zap_pending()
while scmd_eh_abort_handler() -> uas_eh_abort_handler() is actively aborting
cmnd, doesn't clearing COMMAND_ABORTED here allow uas_try_complete() to call
scsi_done(cmnd) while the SCSI error handler still owns the command?

[ ... ]

> @@ -730,35 +738,188 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)

[ ... ]

> +static void uas_dummy_sense(struct urb *u)
> +{
> +	struct scsi_cmnd *cmnd = u->context;
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
> +	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	int tag = cmdinfo->uas_tag;
> +	unsigned long flags;
> +	/*
> +	 * An error implies success
> +	 */
> +	int status = u->status;
> +
> +	/*
> +	 * The tag must be freed in every case
> +	 */
> +	spin_lock_irqsave(&devinfo->lock, flags);
> +	devinfo->cmnd[tag - 1] = NULL;

[Severity: High]
Can clearing devinfo->cmnd[tag - 1] here in uas_dummy_sense() (and in
uas_abort_complete()) allow uas_queuecommand_lck() on another CPU to reuse
the tag and stream ID before uas_eh_abort_handler() has woken up and killed
the old command's in-flight data_in_urb, data_out_urb, and sense URB?

> +	switch (status) {
> +	case -ESHUTDOWN:
> +	case -ENOENT:
> +	case -ECONNRESET:
> +	case 0:
> +		cmdinfo->cleanly_cancelled = 1;
> +		break;
> +	}
> +	spin_unlock_irqrestore(&devinfo->lock, flags);
> +
> +	kfree(u->transfer_buffer);
> +	wake_up_all(&cmdinfo->cancel);
> +}

[Severity: High]
When the status-stage killer URB is unlinked or killed on the host (for
example via usb_kill_anchored_urbs(&devinfo->killer_urbs) in uas_disconnect()
or uas_eh_host_reset_handler()), u->status is -ENOENT, -ECONNRESET, or
-ESHUTDOWN.

Why does uas_dummy_sense() set cmdinfo->cleanly_cancelled = 1 for these host
cancellation error codes? If the URB is killed before schedule_timeout(HZ)
expires in uas_eh_abort_handler(), won't uas_eh_abort_handler() see rv > 0
and cmdinfo->cleanly_cancelled == 1, returning SUCCESS to the SCSI error
handler when the command was not aborted on the target?

> +
> +static void uas_abort_complete(struct urb *u)
> +{
> +	int status = u->status;
> +	struct scsi_cmnd *cmnd = u->context;
> +	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
> +	struct usb_device *udev = devinfo->udev;
> +	struct response_iu *dummy;
> +	int tag = cmdinfo->uas_tag;
> +	unsigned long flags;
> +	int rv;
> +
> +	switch (status) {
> +	default:
> +		/*
> +		 * immediate cleanup
> +		 */
> +		kfree(u->transfer_buffer);
> +		spin_lock_irqsave(&devinfo->lock, flags);
> +		devinfo->cmnd[tag - 1] = NULL;
> +		spin_unlock_irqrestore(&devinfo->lock, flags);
> +		wake_up_all(&cmdinfo->cancel);
> +		break;
> +	case 0:
> +		kfree(u->transfer_buffer);
> +		dummy = kzalloc(sizeof(*dummy), GFP_ATOMIC);
> +		if (!dummy) {
> +			spin_lock_irqsave(&devinfo->lock, flags);
> +			goto bailout;
> +		}
> +		usb_fill_bulk_urb(cmdinfo->killer, udev, devinfo->status_pipe,
> +				dummy, sizeof(*dummy), uas_dummy_sense, cmnd);
> +		spin_lock_irqsave(&devinfo->lock, flags);
> +		if (devinfo->use_streams)
> +			cmdinfo->killer->stream_id = cmdinfo->uas_tag;
> +		usb_anchor_urb(u, &devinfo->killer_urbs);
> +		rv = usb_submit_urb(cmdinfo->killer, GFP_ATOMIC);

[Severity: High]
Can this dereference a NULL cmdinfo->killer when uas_abort_complete() races
with a timeout in uas_eh_abort_handler()?

When schedule_timeout(HZ) expires in uas_eh_abort_handler(), it clears
cmdinfo->killer = NULL under devinfo->lock before calling
usb_kill_urb(killer):

    spin_lock_irq(&devinfo->lock);
    cmdinfo->killer = NULL;
futile:
    spin_unlock_irq(&devinfo->lock);
    usb_kill_urb(killer);

If the command-pipe killer URB completes with status == 0 on another CPU just
before usb_kill_urb(killer) unlinks it, uas_abort_complete() passes
cmdinfo->killer to usb_fill_bulk_urb() without holding devinfo->lock and then
dereferences cmdinfo->killer->stream_id and usb_submit_urb(cmdinfo->killer,
GFP_ATOMIC). Should uas_abort_complete() use the callback parameter u instead
of cmdinfo->killer?

[Severity: High]
How does submitting a second URB on devinfo->status_pipe in
uas_abort_complete() interact with the command's existing sense URB?

When a command is queued in uas_submit_urbs() -> uas_submit_sense_urb(), a
sense URB with completion handler uas_stat_cmplt() is already queued on
devinfo->status_pipe (and on stream_id = cmdinfo->uas_tag when streams are
enabled).

1. When the target detects the overlapped tag and sends a single Response IU
(RC_OVERLAPPED_TAG), won't the original sense URB at the head of status_pipe
consume it in uas_stat_cmplt()? In uas_stat_cmplt(), uas_try_complete()
returns -EBUSY because COMMAND_ABORTED is set, without setting
cmdinfo->cleanly_cancelled or waking cmdinfo->cancel, while the killer URB
queued behind it never completes and schedule_timeout(HZ) in
uas_eh_abort_handler() times out.

2. If the original command completed just before the duplicate
TEST_UNIT_READY arrived, the target will return a Sense IU (at least 16
bytes) for TEST_UNIT_READY into dummy, which is allocated as sizeof(struct
response_iu) (8 bytes), failing with -EOVERFLOW.

3. On USB 2.0 (!devinfo->use_streams), status_pipe is shared across all tags,
and uas_dummy_sense() does not check the tag or IU ID in u->transfer_buffer,
so could it consume a status IU belonging to an unrelated command?

4. What kills the original sense URB anchored in devinfo->sense_urbs when a
command is aborted?

> +		if (rv < 0) {
> +bailout:
> +			devinfo->cmnd[tag - 1] = NULL;
> +			spin_unlock_irqrestore(&devinfo->lock, flags);
> +			usb_unanchor_urb(u);
> +			kfree(dummy);
> +			usb_free_urb(u);
> +		} else {
> +			spin_unlock_irqrestore(&devinfo->lock, flags);
> +		}
> +		break;
> +	}
> +}

[Severity: Critical]
Does jumping to bailout in uas_abort_complete() cause a use-after-free and
double-free of the killer URB?

In uas_eh_abort_handler(), killer is allocated with usb_alloc_urb() (refcount
1). When the first submission completes and __usb_hcd_giveback_urb() calls
uas_abort_complete(u), if kzalloc() fails or usb_submit_urb() returns an
error (for example because usb_kill_urb() or usb_kill_anchored_urbs() marked
the URB as rejected), the bailout path unanchors u and calls usb_free_urb(u).

That drops uas_eh_abort_handler()'s reference while __usb_hcd_giveback_urb()
is still running, so the URB is freed as soon as uas_abort_complete()
returns. Afterwards, uas_eh_abort_handler() (after waiting the full HZ
because bailout also omits wake_up_all(&cmdinfo->cancel)) calls
usb_kill_urb(killer) and usb_free_urb(killer) on the already-freed URB.

> +
> +static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
> +	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	struct usb_device *udev = devinfo->udev;
> +	struct urb *data_in_urb = NULL;
> +	struct urb *data_out_urb = NULL;
> +	struct urb *killer = NULL;
> +	struct command_iu *bullet = NULL;
> +	int target_tag, rv;
> +	int result = FAILED;
> +
> +	killer = usb_alloc_urb(0, GFP_NOIO);
> +
> +	set_current_state(TASK_UNINTERRUPTIBLE);
> +	add_wait_queue(&cmdinfo->cancel, &wait);
> +	spin_lock_irq(&devinfo->lock);
> +	if (devinfo->resetting)
> +		goto futile;
> +	target_tag = cmdinfo->uas_tag;
>  
>  	/* Ensure that try_complete does not call scsi_done */
>  	cmdinfo->state |= COMMAND_ABORTED;

[Severity: High]
What happens if cmnd completed via uas_try_complete() in the window between
the block layer timeout and uas_eh_abort_handler() acquiring devinfo->lock?

In that window, uas_try_complete() clears devinfo->cmnd[cmdinfo->uas_tag - 1]
= NULL and frees unsubmitted URBs, and uas_queuecommand_lck() may have
already assigned target_tag to a newly queued command. Since
uas_eh_abort_handler() does not check whether devinfo->cmnd[target_tag - 1]
== cmnd under devinfo->lock, won't it send a duplicate-tag abort for a tag
now owned by a different command (or clear that new command's slot in
devinfo->cmnd[] and double-free unsubmitted URBs on the admit_failure path)?

> -	/* Drop all refs to this cmnd, kill data urbs to break their ref */
> -	devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
> -	if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> -		data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> -	if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> -		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
> -
> -	uas_free_unsubmitted_urbs(cmnd);
> +	if (!killer)
> +		goto admit_failure;
> +	cmdinfo->killer = killer;
> +	bullet = allocate_double_command(target_tag, cmnd->device);
> +	if (!bullet)
> +		goto admit_failure;
> +
> +	usb_fill_bulk_urb(killer, udev, devinfo->cmd_pipe,
> +			bullet, sizeof(*bullet), uas_abort_complete, cmnd);
> +
> +	cmdinfo->killer = killer;
> +
> +	usb_anchor_urb(killer, &devinfo->killer_urbs);
> +	rv = usb_submit_urb(killer, GFP_ATOMIC);
> +	if (rv < 0) {
> +admit_failure:
> +		/* Drop all refs to this cmnd, kill data urbs to break their ref */
> +		devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
> +		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> +			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> +		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> +			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
>  
> -	spin_unlock_irqrestore(&devinfo->lock, flags);
> +		uas_free_unsubmitted_urbs(cmnd);
> +		spin_unlock_irq(&devinfo->lock);
> +		usb_unanchor_urb(killer);
> +		kfree(bullet);
> +	} else {
> +		spin_unlock_irq(&devinfo->lock);
> +		rv = schedule_timeout(HZ);
> +		if (rv)
> +			result = cmdinfo->cleanly_cancelled ? SUCCESS : FAILED;
> +		spin_lock_irq(&devinfo->lock);
> +		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> +			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> +		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> +			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
> +		spin_unlock_irq(&devinfo->lock);
> +	}

[Severity: High]
Does the else branch leak unsubmitted URBs (cmdinfo->cmd_urb,
cmdinfo->data_in_urb, and cmdinfo->data_out_urb)?

When usb_submit_urb(killer, GFP_ATOMIC) succeeds, uas_free_unsubmitted_urbs()
is never called on this path, and uas_try_complete() will not free them
because COMMAND_ABORTED is set. On USB 2.0 (!devinfo->use_streams) where data
URBs are allocated up front but only submitted upon receiving
IU_ID_READ_READY or IU_ID_WRITE_READY (or when uas_do_work() is pending), any
unsubmitted URBs on cmnd appear to be leaked.

Also, if schedule_timeout(HZ) times out (rv == 0), devinfo->cmnd[target_tag -
1] is still non-NULL when DATA_IN_URB_INFLIGHT and DATA_OUT_URB_INFLIGHT are
checked above, and IS_IN_WORK_LIST is not cleared. Could uas_do_work() or
uas_stat_cmplt() submit data_in_urb or data_out_urb after this check and
leave a data URB in flight after uas_eh_abort_handler() returns?

> +	set_current_state(TASK_RUNNING);
> +	remove_wait_queue(&cmdinfo->cancel, &wait);
> +	/*
> +	 * we know a callback will free the tag
> +	 */
> +	spin_lock_irq(&devinfo->lock);
> +	cmdinfo->killer = NULL;
> +futile:
> +	spin_unlock_irq(&devinfo->lock);
> +	usb_kill_urb(killer);
> +	usb_free_urb(killer);

[Severity: Critical]
If devinfo->resetting is true when uas_eh_abort_handler() acquires
devinfo->lock, jumping to futile skips set_current_state(TASK_RUNNING) and
remove_wait_queue(&cmdinfo->cancel, &wait).

Doesn't that call usb_kill_urb(killer) while current is still in
TASK_UNINTERRUPTIBLE state, and then return from uas_eh_abort_handler() with
the stack-allocated wait entry still linked on cmdinfo->cancel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/38f81744-9122-4109-90b6-b0a1ac92d248@suse.com?part=1

Re: [RFC]alpha implementation of eh_abort_handler for UAS

[Jiayi Li]

Hi,

On Thu, Nov 05, 2026 at 08:33:33PM +0100, Oliver Neukum wrote:
> the older version had serious bugs.

I tested RFCv2 and the local changes described below on 7.0.0-38-generic
with USB streams enabled. The host_lock accesses were adapted to that
kernel's API. All tag numbers below refer to UAS IU tags.

1) RC_OVERLAPPED_TAG can reach the original Status receiver

On ZX1, I submitted READ(tag2) through a test helper, cancelled its Data
and Status URBs after its Command OUT URB completed successfully, and
kept the interface and streams unchanged. A subsequent SCSI READ(tag1)
timed out, and uas_eh_abort_handler() automatically submitted TUR(tag1).

During the abort wait, RC_OVERLAPPED_TAG reached the original 112-byte
Status receiver. The additional 8-byte receiver remained pending and
was subsequently cancelled.

uas_stat_cmplt() processed the Response IU but did not notify
cmdinfo->cancel, so abort continued waiting and returned FAILED. For
subsequent tests, I added this notification to its IU_ID_RESPONSE path:


  if ((cmdinfo->state & COMMAND_ABORTED) &&
       ((struct response_iu *)iu)->response_code == RC_OVERLAPPED_TAG) {
	  cmdinfo->cleanly_cancelled = 1;
	  wake_up_all(&cmdinfo->cancel);
  }

The locally modified RFCv2 also uses u rather than cmdinfo->killer for
the follow-on Status submission in uas_abort_complete(), and leaves
final URB freeing to uas_eh_abort_handler(), including on submission
failure.

2) Recovery after read cancellation: single and concurrent requests

These tests used the Ugreen enclosure (0bda:9210) with an SK hynix BC501
and the locally modified RFCv2 described above. The filesystems were
unmounted, and reads used the normal block/SCSI path with O_DIRECT.

For a single request, I cancelled the Data and Status URBs of one
512 KiB READ and waited for recovery before issuing further reads.
EH submitted a same-tag TUR, RC_OVERLAPPED_TAG reached the additional
Status receiver through uas_dummy_sense(), and abort returned SUCCESS
without a USB reset. A new 4 MiB read and 128 further 512 KiB reads
matched the reference data.

For concurrent requests, I ran four workers issuing 512 KiB reads,
cancelled one READ's Data and Status URBs, and immediately submitted
another 4 MiB read, which was split into eight SCSI READ commands.
I repeated this twice.

Each run had 12 calls to uas_eh_abort_handler(): one returned SUCCESS
after RC_OVERLAPPED_TAG reached the original 112-byte Status receiver,
and 11 returned FAILED. One automatic USB reset followed. The added
4 MiB read and 128 further 512 KiB reads passed data validation in
both runs.

3) Actual driver unbind/rebind results

I started concurrent block-device reads, confirmed pending Data URBs
at usb_unbind_interface() entry, and then unbound and rebound UAS
through sysfs.

On ZX1 512GB (2109:0715), using RFCv2 with only the host_lock adaptation,
the first command after rebind was INQUIRY with tag1 and a 36-byte
allocation length. Its Data-IN URB returned -75 with actual_length=0.
EH subsequently submitted TUR(tag1), whose Command OUT URB completed
successfully, but RC_OVERLAPPED_TAG was not observed. Subsequent reads
did not recover.

With the Ugreen/BC501 combination and the locally modified RFCv2,
I repeated six rebind tests. Every run had 30 pending Data URBs at
usb_unbind_interface() entry, and the corresponding READ Command OUT
URBs had already completed successfully.

Four runs completed without EH. In the other two, a READ at LBA0 during
post-rebind device discovery timed out. EH submitted TUR(tag1), and the
original 112-byte Status receiver received RC_OVERLAPPED_TAG. Abort
returned SUCCESS without a USB reset. Three subsequent 4 MiB reads
matched the reference data in each of the six runs.

RC_OVERLAPPED_TAG was not observed in the ZX1 rebind case, and I have
not yet established why.


Thanks,
Jiayi