Re: [RFC]preliminary support for eh_abort_handler

[sashiko-bot@kernel.org]

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