* Re: [RFC]preliminary support for eh_abort_handler
2026-11-05 12:51 [RFC]preliminary support for eh_abort_handler Oliver Neukum
@ 2026-11-05 13:08 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-05 13:08 UTC (permalink / raw)
To: Oliver Neukum; +Cc: USB list
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] uas: NULL dereference of cmdinfo->killer in uas_abort_complete()
- [Critical] uas: premature tag release and racing URB submissions during abort
- [Critical] uas: double free and wrong-tag abort in uas_eh_abort_handler()
- [High] uas: duplicate status URB in uas_abort_complete() conflicts with sense URB
- [High] uas: URB unlink statuses treated as success in abort callbacks
- [High] uas: remove_wait_queue() on unadded entry in uas_eh_abort_handler()
- [Medium] uas: memory leak of dummy buffer in uas_abort_complete() on submit error
- [Medium] uas: missing uas_free_unsubmitted_urbs() in uas_eh_abort_handler()
- [Medium] uas: premature TASK_UNINTERRUPTIBLE before lock and printk in abort
--
commit 011b29f56a91695e69452114ed53025593de9c85
Author: Oliver Neukum <oneukum@suse.com>
preliminary support for eh_abort_handler
This commit implements uas_eh_abort_handler() by sending a duplicate
TEST_UNIT_READY command with the same tag as the command being aborted to
trigger an overlapped-tag error on the target.
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..d7421bf82209e 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: Critical]
Can this cause a double free of unsubmitted URBs or abort a newly queued
command if a command completes or is zapped before or during
uas_eh_abort_handler()?
Before uas_eh_abort_handler() acquires devinfo->lock (including while
usb_alloc_urb(0, GFP_NOIO) runs), cmnd may have already completed via
uas_try_complete() or been zapped by uas_zap_pending() during
uas_disconnect() or uas_eh_host_reset_handler().
In uas_try_complete(), devinfo->cmnd[cmdinfo->uas_tag - 1] is cleared to
NULL and uas_free_unsubmitted_urbs(cmnd) frees cmdinfo->cmd_urb,
cmdinfo->data_in_urb, and cmdinfo->data_out_urb without clearing those
pointers:
static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
{
...
devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
uas_free_unsubmitted_urbs(cmnd);
scsi_done(cmnd);
return 0;
}
Because uas_eh_abort_handler() does not check devinfo->resetting or verify
that devinfo->cmnd[target_tag - 1] == cmnd after acquiring devinfo->lock,
if killer URB submission fails and jumps to admit_failure,
uas_eh_abort_handler() calls uas_free_unsubmitted_urbs(cmnd) a second time
on the dangling URB pointers.
Additionally, if target_tag was already reallocated to a new command by
uas_queuecommand_lck(), uas_eh_abort_handler() will send a duplicate-tag
command for target_tag and clear devinfo->cmnd[target_tag - 1], aborting
and corrupting the new command. Likewise, if uas_zap_pending() runs while
uas_eh_abort_handler() is sleeping in schedule_timeout(HZ), clearing
COMMAND_ABORTED here allows uas_zap_pending() to call uas_try_complete() on
cmnd while uas_eh_abort_handler() is still operating on it.
[ ... ]
> @@ -730,35 +738,182 @@ 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;
[Severity: Critical]
Does clearing devinfo->cmnd[tag - 1] here and in uas_abort_complete() race
with in-flight and late-submitted data URBs?
scmd_eh_abort_handler() invokes uas_eh_abort_handler() while the Scsi_Host
is still in SHOST_RUNNING, so uas_queuecommand_lck() can run concurrently
on another CPU. When uas_dummy_sense() or uas_abort_complete() sets
devinfo->cmnd[tag - 1] = NULL before uas_eh_abort_handler() wakes up and
kills data_in_urb and data_out_urb (and also on bailout or admit_failure
when the target may still be executing the command on that tag), a new
command in uas_queuecommand_lck() can immediately allocate tag - 1 and
submit URBs on the same stream_id while the aborted command's URBs are
still active.
Conversely, while devinfo->cmnd[tag - 1] remains non-NULL during
schedule_timeout(HZ) (and on timeout, even after uas_eh_abort_handler()
checks DATA_IN_URB_INFLIGHT and DATA_OUT_URB_INFLIGHT before
usb_kill_urb(killer) triggers the completion callback that clears
devinfo->cmnd[tag - 1]), neither uas_do_work() nor uas_stat_cmplt() (when
receiving IU_ID_READ_READY or IU_ID_WRITE_READY) checks COMMAND_ABORTED:
static void uas_do_work(struct work_struct *work)
{
...
if (!(cmdinfo->state & IS_IN_WORK_LIST))
continue;
err = uas_submit_urbs(cmnd, cmnd->device->hostdata);
...
}
Can uas_do_work() or uas_stat_cmplt() -> uas_xfer_data() submit
data_in_urb or data_out_urb after uas_eh_abort_handler() has already
checked DATA_IN_URB_INFLIGHT and DATA_OUT_URB_INFLIGHT, leaving active data
URBs with urb->context = cmnd in flight after uas_eh_abort_handler()
returns?
> + switch (status) {
> + case -ESHUTDOWN:
> + case -ENOENT:
> + case -ECONNRESET:
> + case 0:
> + cmdinfo->cleanly_cancelled = 1;
> + break;
> + }
[Severity: High]
Should -ESHUTDOWN, -ENOENT, and -ECONNRESET really be treated as successful
completions in uas_dummy_sense() and uas_abort_complete()?
Those status codes indicate that the URB was unlinked or killed on the host
side (for example via usb_kill_urb(killer) on timeout in
uas_eh_abort_handler(), or via usb_kill_anchored_urbs(&devinfo->killer_urbs)
in uas_eh_host_reset_handler() and uas_disconnect()), not that the target
responded.
In uas_dummy_sense(), setting cmdinfo->cleanly_cancelled = 1 on unlink
means that if usb_kill_anchored_urbs(&devinfo->killer_urbs) kills the URB
while uas_eh_abort_handler() has remaining timeout (rv > 0),
uas_eh_abort_handler() will wake up and return SUCCESS even though the
abort URB was killed rather than completed.
In uas_abort_complete(), grouping -ECONNRESET, -ESHUTDOWN, and -ENOENT with
case 0 causes a killed command-pipe URB to allocate dummy and resubmit
cmdinfo->killer onto devinfo->status_pipe instead of bailing out.
> + 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);
> +bailout:
> + 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:
> + case -ECONNRESET:
> + case -ESHUTDOWN:
> + case -ENOENT:
> + kfree(u->transfer_buffer);
> + dummy = kzalloc(sizeof(*dummy), GFP_ATOMIC);
> + if (!dummy)
> + 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]
How does submitting cmdinfo->killer on devinfo->status_pipe interact with
the sense URB that was already submitted for this command in
uas_submit_urbs() -> uas_submit_sense_urb()?
When cmnd was originally queued, uas_submit_sense_urb() already queued a
sense URB (with completion callback uas_stat_cmplt()) on
devinfo->status_pipe for this stream_id. When the duplicate TEST_UNIT_READY
triggers an overlapped tag error, the target sends a single IU_ID_RESPONSE
(RC_OVERLAPPED_TAG). Because URBs on the same pipe/stream complete in FIFO
order, that Response IU will be received by the original sense URB at the
head of the queue in uas_stat_cmplt(), where uas_try_complete() sees
COMMAND_ABORTED and returns -EBUSY without waking cmdinfo->cancel:
case IU_ID_RESPONSE:
cmdinfo->state &= ~COMMAND_INFLIGHT;
success = uas_evaluate_response_iu((struct response_iu *)iu, cmnd);
if (!success) {
/* Error, cancel data transfers */
data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
}
uas_try_complete(cmnd, __func__);
break;
Meanwhile, cmdinfo->killer is queued behind the original sense URB waiting
for a second status IU that never arrives, so schedule_timeout(HZ) in
uas_eh_abort_handler() will time out and return FAILED, while leaving the
original sense URB dangling on devinfo->sense_urbs if it did not complete.
Also, if the original command finished just before the duplicate
TEST_UNIT_READY arrived, the target will execute TEST_UNIT_READY and return
a Sense IU (at least 16 bytes) into dummy, which is only
sizeof(struct response_iu) (8 bytes), causing -EOVERFLOW. And on USB 2.0
(!devinfo->use_streams), queuing cmdinfo->killer with uas_dummy_sense() on
the shared status_pipe without checking iu->tag or iu->iu_id can consume
the next status or ready IU belonging to an unrelated command on another
tag.
> + spin_unlock_irqrestore(&devinfo->lock, flags);
> + if (rv < 0) {
> + usb_unanchor_urb(u);
> + goto bailout;
> + }
[Severity: Medium]
If usb_submit_urb(cmdinfo->killer, GFP_ATOMIC) fails here (rv < 0), does
jumping to bailout leak the newly allocated dummy buffer?
The bailout label is placed after kfree(u->transfer_buffer), and
uas_eh_abort_handler() only calls usb_kill_urb(killer) and
usb_free_urb(killer) without freeing killer->transfer_buffer.
> + }
> +}
> +
> 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);
> + spin_lock_irq(&devinfo->lock);
[Severity: Medium]
Is it safe to call set_current_state(TASK_UNINTERRUPTIBLE) before
spin_lock_irq(&devinfo->lock), uas_log_cmd_state(), and add_wait_queue()?
uas_log_cmd_state() calls scmd_printk() and scsi_print_command(), and on
PREEMPT_RT spin_lock_irq() acquires a sleeping rt_mutex. Calling these with
current->__state set to TASK_UNINTERRUPTIBLE can trigger might_sleep()
warnings and reset current->__state back to TASK_RUNNING before
schedule_timeout(HZ) is reached, causing schedule_timeout(HZ) to return
immediately with rv > 0 while cmdinfo->cleanly_cancelled is still 0.
> + 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);
> + if (!killer)
> + goto admit_failure;
> + cmdinfo->killer = killer;
> + bullet = allocate_double_command(target_tag, cmnd->device);
> + if (!bullet)
> + goto admit_failure;
>
> - uas_free_unsubmitted_urbs(cmnd);
> + usb_fill_bulk_urb(killer, udev, devinfo->cmd_pipe,
> + bullet, sizeof(*bullet), uas_abort_complete, cmnd);
> + uas_log_cmd_state(cmnd, __func__, 0);
>
> - spin_unlock_irqrestore(&devinfo->lock, flags);
> + cmdinfo->killer = killer;
> + add_wait_queue(&cmdinfo->cancel, &wait);
> +
> + 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);
> +
> + 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: Medium]
Does the else branch here leak unsubmitted URBs by omitting
uas_free_unsubmitted_urbs(cmnd)?
Because COMMAND_ABORTED is set on cmdinfo->state, uas_try_complete()
returns -EBUSY without calling uas_free_unsubmitted_urbs(cmnd), and
devinfo->cmnd[tag - 1] is cleared to NULL by the abort callbacks. When
usb_submit_urb(killer, GFP_ATOMIC) succeeds, any allocated but unsubmitted
URBs (such as data_in_urb or data_out_urb on USB 2.0 before
IU_ID_READ_READY or IU_ID_WRITE_READY arrives, or URBs pending on
devinfo->work) are never freed.
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&cmdinfo->cancel, &wait);
[Severity: High]
What happens in remove_wait_queue(&cmdinfo->cancel, &wait) if killer or
bullet allocation failed earlier?
When !killer or !bullet jumps to admit_failure, add_wait_queue() is
skipped, leaving wait.entry.next and wait.entry.prev as NULL. Calling
remove_wait_queue(&cmdinfo->cancel, &wait) unconditionally here will
dereference NULL in list_del(&wait.entry).
> + /*
> + * we know a callback will free the tag
> + */
> + spin_lock_irq(&devinfo->lock);
> + cmdinfo->killer = NULL;
> + spin_unlock_irq(&devinfo->lock);
> + usb_kill_urb(killer);
[Severity: Critical]
Can setting cmdinfo->killer = NULL before usb_kill_urb(killer) cause a NULL
pointer dereference in uas_abort_complete()?
If schedule_timeout(HZ) times out while the first-stage killer URB is still
in flight on devinfo->cmd_pipe (or completing concurrently on another CPU),
cmdinfo->killer is set to NULL here and then usb_kill_urb(killer) unlinks
the URB with status -ENOENT and waits for uas_abort_complete() to finish.
In uas_abort_complete(), both status == 0 and status == -ENOENT enter the
resubmit path and dereference cmdinfo->killer without checking if it is
NULL:
case 0:
case -ECONNRESET:
case -ESHUTDOWN:
case -ENOENT:
kfree(u->transfer_buffer);
dummy = kzalloc(sizeof(*dummy), GFP_ATOMIC);
if (!dummy)
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);
Wouldn't usb_fill_bulk_urb(cmdinfo->killer, ...) immediately dereference
NULL?
--
Sashiko AI review · https://sashiko.dev/#/patchset/4fdff241-7092-4013-8af1-89ceb85f2b14@suse.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread