[PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hui Wang]

Once pre_reset() or post_reset() returns non-zero, the disconnect()
and probe() of the usb_driver will be called. In the disconnect(),
the scsi_host will be removed and be freed after scsi_host_put(), in
the probe(), the new scsi_host and uas_dev_info will be created.

If the usb_reset_device() is triggered by eh_device_reset_handler(),
and pre_reset()/post_reset() returns non-zero, the disconnect() and
probe() will be called, then returns to the eh_device_reset_handler(),
it still accesses old scsi related variables and uas_dev_info, and so
do its caller functions.

Here change the pre_reset() and post_reset() to let them only return
0, after this change, the usb_reset_device() will only reset this
usb devcie from its hub port, will not execute unbind and rebind
usb_driver during reset.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/usb/storage/uas.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index bef89c6bd1d7..c66287448e34 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1121,7 +1121,6 @@ static int uas_pre_reset(struct usb_interface *intf)
 	if (uas_wait_for_pending_cmnds(devinfo) != 0) {
 		shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
 		scsi_unblock_requests(shost);
-		return 1;
 	}
 
 	uas_free_streams(devinfo);
@@ -1152,7 +1151,7 @@ static int uas_post_reset(struct usb_interface *intf)
 
 	scsi_unblock_requests(shost);
 
-	return err ? 1 : 0;
+	return 0;
 }
 
 static int uas_suspend(struct usb_interface *intf, pm_message_t message)
-- 
2.25.1

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hans de Goede]

Hi,

On 2/21/21 9:51 AM, Hui Wang wrote:
> Once pre_reset() or post_reset() returns non-zero, the disconnect()
> and probe() of the usb_driver will be called. In the disconnect(),
> the scsi_host will be removed and be freed after scsi_host_put(), in
> the probe(), the new scsi_host and uas_dev_info will be created.
> 
> If the usb_reset_device() is triggered by eh_device_reset_handler(),
> and pre_reset()/post_reset() returns non-zero, the disconnect() and
> probe() will be called, then returns to the eh_device_reset_handler(),
> it still accesses old scsi related variables and uas_dev_info, and so
> do its caller functions.
> 
> Here change the pre_reset() and post_reset() to let them only return
> 0, after this change, the usb_reset_device() will only reset this
> usb devcie from its hub port, will not execute unbind and rebind
> usb_driver during reset.

We only return non 0 from the pre/post reset handles if we failed
to ensure the device is in a known state.

E.g. in uas_post_reset() we only fail if we failed to switch the
device from the good old usb-storage protocol back to the UAS mode
which it was in before.

Continuing with the UAS driver bound, as if everything is fine,
while the device is actually in longer in UAS mode is a bad idea.

Summarizing this patch is wrong: NACK.

###

I assume that you wrote this patch because of some bug report ?

In such a case please always include a link to the bug (or forum
discussion) in the commit message.

UAS problems typically are caused by people shoving a 2.5 inch
or m2 SSD in an USB enclosure which is powered from the USB bus
only. SSD-s can cause pretty hefty power-consumption peaks when
high queue depts are used; and these bus powered devices typically
cannot handle these peaks. Either because the port they are
plugged in does not deliver enough current; and/or because they
don't have enough buffering (capacitors) on the enclosure's PCB
to deal with short but quite large consumption peaks.

Regards,

Hans








> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  drivers/usb/storage/uas.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index bef89c6bd1d7..c66287448e34 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -1121,7 +1121,6 @@ static int uas_pre_reset(struct usb_interface *intf)
>  	if (uas_wait_for_pending_cmnds(devinfo) != 0) {
>  		shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
>  		scsi_unblock_requests(shost);
> -		return 1;
>  	}
>  
>  	uas_free_streams(devinfo);
> @@ -1152,7 +1151,7 @@ static int uas_post_reset(struct usb_interface *intf)
>  
>  	scsi_unblock_requests(shost);
>  
> -	return err ? 1 : 0;
> +	return 0;
>  }
>  
>  static int uas_suspend(struct usb_interface *intf, pm_message_t message)
> 

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hui Wang]

On 2/21/21 6:20 PM, Hans de Goede wrote:
> Hi,
>
> On 2/21/21 9:51 AM, Hui Wang wrote:
>> Once pre_reset() or post_reset() returns non-zero, the disconnect()
>> and probe() of the usb_driver will be called. In the disconnect(),
>> the scsi_host will be removed and be freed after scsi_host_put(), in
>> the probe(), the new scsi_host and uas_dev_info will be created.
>>
>> If the usb_reset_device() is triggered by eh_device_reset_handler(),
>> and pre_reset()/post_reset() returns non-zero, the disconnect() and
>> probe() will be called, then returns to the eh_device_reset_handler(),
>> it still accesses old scsi related variables and uas_dev_info, and so
>> do its caller functions.
>>
>> Here change the pre_reset() and post_reset() to let them only return
>> 0, after this change, the usb_reset_device() will only reset this
>> usb devcie from its hub port, will not execute unbind and rebind
>> usb_driver during reset.
> We only return non 0 from the pre/post reset handles if we failed
> to ensure the device is in a known state.
>
> E.g. in uas_post_reset() we only fail if we failed to switch the
> device from the good old usb-storage protocol back to the UAS mode
> which it was in before.
>
> Continuing with the UAS driver bound, as if everything is fine,
> while the device is actually in longer in UAS mode is a bad idea.
>
> Summarizing this patch is wrong: NACK.
>
> ###
>
> I assume that you wrote this patch because of some bug report ?
>
> In such a case please always include a link to the bug (or forum
> discussion) in the commit message.

OK, got it, so far there is no bug about it. I just wrote some code and 
called usb_reset_device(), found it will trigger to call disconnect() 
and probe() in some cases. I found uas.c could trigger to call 
disconnect() and probe() in eh_device_reset_handler(), and could 
possibility introduce use-after-free issue. I thought resetting the 
device from the hub port is enough to let it back to normal work mode, 
if it doesn't, calling disconnect() and probe() will not help either. 
And I checked some eh_device_reset_handler callback in other drivers, 
looks like they don't trigger unbind and rebind, then I wrote this patch.

Thanks,

Hui.

>
> UAS problems typically are caused by people shoving a 2.5 inch
> or m2 SSD in an USB enclosure which is powered from the USB bus
> only. SSD-s can cause pretty hefty power-consumption peaks when
> high queue depts are used; and these bus powered devices typically
> cannot handle these peaks. Either because the port they are
> plugged in does not deliver enough current; and/or because they
> don't have enough buffering (capacitors) on the enclosure's PCB
> to deal with short but quite large consumption peaks.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   drivers/usb/storage/uas.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
>> index bef89c6bd1d7..c66287448e34 100644
>> --- a/drivers/usb/storage/uas.c
>> +++ b/drivers/usb/storage/uas.c
>> @@ -1121,7 +1121,6 @@ static int uas_pre_reset(struct usb_interface *intf)
>>   	if (uas_wait_for_pending_cmnds(devinfo) != 0) {
>>   		shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
>>   		scsi_unblock_requests(shost);
>> -		return 1;
>>   	}
>>   
>>   	uas_free_streams(devinfo);
>> @@ -1152,7 +1151,7 @@ static int uas_post_reset(struct usb_interface *intf)
>>   
>>   	scsi_unblock_requests(shost);
>>   
>> -	return err ? 1 : 0;
>> +	return 0;
>>   }
>>   
>>   static int uas_suspend(struct usb_interface *intf, pm_message_t message)
>>

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Oliver Neukum]

Am Sonntag, den 21.02.2021, 11:20 +0100 schrieb Hans de Goede:
> Hi,

Hi,

> 
> On 2/21/21 9:51 AM, Hui Wang wrote:
> > Once pre_reset() or post_reset() returns non-zero, the disconnect()
> > and probe() of the usb_driver will be called. In the disconnect(),
> > the scsi_host will be removed and be freed after scsi_host_put(), in
> > the probe(), the new scsi_host and uas_dev_info will be created.
> > 
> > If the usb_reset_device() is triggered by eh_device_reset_handler(),
> > and pre_reset()/post_reset() returns non-zero, the disconnect() and
> > probe() will be called, then returns to the eh_device_reset_handler(),
> > it still accesses old scsi related variables and uas_dev_info, and so
> > do its caller functions.
> > 
> > Here change the pre_reset() and post_reset() to let them only return
> > 0, after this change, the usb_reset_device() will only reset this
> > usb devcie from its hub port, will not execute unbind and rebind
> > usb_driver during reset.
> 
> We only return non 0 from the pre/post reset handles if we failed
> to ensure the device is in a known state.

correct. Technically it is a bit unfortunate that UAS devices react
a bit different to other SCSI devices, but we definitely cannot hide
a failure. Arguably we should go into OFFLINE state.
But that needs a
good reason beyond theoretical considerations.

	Regards
		Oliver

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hui Wang]

On 2/22/21 3:59 PM, Oliver Neukum wrote:
> Am Sonntag, den 21.02.2021, 11:20 +0100 schrieb Hans de Goede:
>> Hi,
> Hi,
>
>> On 2/21/21 9:51 AM, Hui Wang wrote:
>>> Once pre_reset() or post_reset() returns non-zero, the disconnect()
>>> and probe() of the usb_driver will be called. In the disconnect(),
>>> the scsi_host will be removed and be freed after scsi_host_put(), in
>>> the probe(), the new scsi_host and uas_dev_info will be created.
>>>
>>> If the usb_reset_device() is triggered by eh_device_reset_handler(),
>>> and pre_reset()/post_reset() returns non-zero, the disconnect() and
>>> probe() will be called, then returns to the eh_device_reset_handler(),
>>> it still accesses old scsi related variables and uas_dev_info, and so
>>> do its caller functions.
>>>
>>> Here change the pre_reset() and post_reset() to let them only return
>>> 0, after this change, the usb_reset_device() will only reset this
>>> usb devcie from its hub port, will not execute unbind and rebind
>>> usb_driver during reset.
>> We only return non 0 from the pre/post reset handles if we failed
>> to ensure the device is in a known state.
> correct. Technically it is a bit unfortunate that UAS devices react
> a bit different to other SCSI devices, but we definitely cannot hide
> a failure. Arguably we should go into OFFLINE state.
> But that needs a
> good reason beyond theoretical considerations.
>
> 	Regards
> 		Oliver
>
>
OK, will find a UAS device to do the test.

thanks,

Hui.

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Oliver Neukum]

Am Montag, den 22.02.2021, 20:40 +0800 schrieb Hui Wang:
> On 2/22/21 3:59 PM, Oliver Neukum wrote:
> > 
> OK, will find a UAS device to do the test.

Hi,

do you have a design at all?

	Regards
		Oliver

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hui Wang]

On 2/22/21 8:51 PM, Oliver Neukum wrote:
> Am Montag, den 22.02.2021, 20:40 +0800 schrieb Hui Wang:
>> On 2/22/21 3:59 PM, Oliver Neukum wrote:
>> OK, will find a UAS device to do the test.
> Hi,
>
> do you have a design at all?

No, so far what I could find is all driven by usb-storage, I tested a 
couple of usb-sdcard-readers and usb-scsi/ata disk adapters, they all 
belong to USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, 
USB_PR_BULK) instead of USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, 
USB_SC_SCSI, USB_PR_UAS). I plan to go to the office to find some usb 
storage devices to test.

Regards,

Hui.

>
> 	Regards
> 		Oliver
>
>

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Oliver Neukum]

Am Montag, den 22.02.2021, 21:02 +0800 schrieb Hui Wang:
> On 2/22/21 8:51 PM, Oliver Neukum wrote:
> > Am Montag, den 22.02.2021, 20:40 +0800 schrieb Hui Wang:
> > > On 2/22/21 3:59 PM, Oliver Neukum wrote:
> > > OK, will find a UAS device to do the test.
> > Hi,
> > 
> > do you have a design at all?
> 
> No, so far what I could find is all driven by usb-storage, I tested a 
> couple of usb-sdcard-readers and usb-scsi/ata disk adapters, they all 
> belong to USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI, 
> USB_PR_BULK) instead of USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, 
> USB_SC_SCSI, USB_PR_UAS). I plan to go to the office to find some usb 
> storage devices to test.

Hi,

please wait.  First of all, you are making the assumption that all
resets originate from the SCSI layer. You cannot make that assumption.

Secondly, yes, ideally we should not pretend that a disconnect has
happened, when it hasn't happened, but what is your alternative.
What exactly do you want to test? You have not even defined the
desirable behavior and the problem you are seeing with the current
behavior.

	Regards
		Oliver

Re: [PATCH] USB: UAS: don't unbind and rebind the driver during usb_reset_device

[Hui Wang]

On 2/22/21 9:50 PM, Oliver Neukum wrote:
> Am Montag, den 22.02.2021, 21:02 +0800 schrieb Hui Wang:
>> On 2/22/21 8:51 PM, Oliver Neukum wrote:
>>> Am Montag, den 22.02.2021, 20:40 +0800 schrieb Hui Wang:
>>>> On 2/22/21 3:59 PM, Oliver Neukum wrote:
>>>> OK, will find a UAS device to do the test.
>>> Hi,
>>>
>>> do you have a design at all?
>> No, so far what I could find is all driven by usb-storage, I tested a
>> couple of usb-sdcard-readers and usb-scsi/ata disk adapters, they all
>> belong to USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, USB_SC_SCSI,
>> USB_PR_BULK) instead of USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE,
>> USB_SC_SCSI, USB_PR_UAS). I plan to go to the office to find some usb
>> storage devices to test.
> Hi,
>
> please wait.  First of all, you are making the assumption that all
> resets originate from the SCSI layer. You cannot make that assumption.
>
> Secondly, yes, ideally we should not pretend that a disconnect has
> happened, when it hasn't happened, but what is your alternative.
> What exactly do you want to test? You have not even defined the
> desirable behavior and the problem you are seeing with the current
> behavior.

I planed to forcibly (simulate) trigger calling 
eh_device_reset_handler() from scsi layer and let pre_reset() or 
post_reset() return a non-zero, and test if there is use-after-free 
issue in the rest part of eh_device_reset_handler() and its callers. But 
after thinking of your comment, looks like I was wrong. Thanks for your 
instructions on this issue.

Thanks,

Hui.

> 	Regards
> 		Oliver
>
>