race condition between shutdown() and post_reset() resulting in deadlock

race condition between shutdown() and post_reset() resulting in deadlock

[Oliver Neukum]

Hi,

looking at your last patch I noticed something.
I think it fails to work if we hit a peculiar race condition.
It looks to me like we can get the following:

task A

pre_reset()
  --> calling scsi_block_requests()

					task B

					shutdown()
					  --> setting the shutdown flag


post_reset()
  --> checking the flag -> do nothing -> deadlock

So we need in addition something like the below patch.
What do you think?

	Regards
		Oliver

From 68cc8940d7dde8e2a48217d8086cd692809e7980 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Mon, 15 Jan 2018 13:56:59 +0100
Subject: [PATCH] UAS: fix race between post_reset() and shutdown()

Disconnect due to shutdown can happen after pre_reset() has been called. Thus the host
must be unblocked even in the shutdown case.

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

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 338133a239df..d5ed1e581fb5 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1046,10 +1046,11 @@ static int uas_post_reset(struct usb_interface *intf)
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
 	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
 	unsigned long flags;
-	int err;
+	int err = 1;
 
+	/* We may have lost the race and shutdown after pre_reset() */
 	if (devinfo->shutdown)
-		return 0;
+		goto skip;
 
 	err = uas_configure_endpoints(devinfo);
 	if (err && err != ENODEV)
@@ -1062,6 +1063,7 @@ static int uas_post_reset(struct usb_interface *intf)
 	scsi_report_bus_reset(shost, 0);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
+skip:
 	scsi_unblock_requests(shost);
 
 	return err ? 1 : 0;

race condition between shutdown() and post_reset() resulting in deadlock

[Hans de Goede]

Hi,

On 16-01-18 10:57, Oliver Neukum wrote:
> Hi,
> 
> looking at your last patch I noticed something.
> I think it fails to work if we hit a peculiar race condition.
> It looks to me like we can get the following:
> 
> task A
> 
> pre_reset()
>    --> calling scsi_block_requests()
> 
> 					task B
> 
> 					shutdown()
> 					  --> setting the shutdown flag
> 
> 
> post_reset()
>    --> checking the flag -> do nothing -> deadlock
> 
> So we need in addition something like the below patch.
> What do you think?

Interesting theory, I guess you're right that we've a race,
but it seems to me that the proper fix would be to call
usb_lock_device_for_reset() from the shutdown callback,
something which the usb_device_reset docs say we should
have done from day 1...

Note please test this actually works before upstreaming this :)

Also at the point shutdown runs all block queues have been
flushed (including the on disk caches) and no new block io
requests can get queued, so keeping the queue blocked if we
hit the race should not be a problem.

Regards,

Hans


> 
> 	Regards
> 		Oliver
> 
>  From 68cc8940d7dde8e2a48217d8086cd692809e7980 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Mon, 15 Jan 2018 13:56:59 +0100
> Subject: [PATCH] UAS: fix race between post_reset() and shutdown()
> 
> Disconnect due to shutdown can happen after pre_reset() has been called. Thus the host
> must be unblocked even in the shutdown case.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>   drivers/usb/storage/uas.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 338133a239df..d5ed1e581fb5 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -1046,10 +1046,11 @@ static int uas_post_reset(struct usb_interface *intf)
>   	struct Scsi_Host *shost = usb_get_intfdata(intf);
>   	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
>   	unsigned long flags;
> -	int err;
> +	int err = 1;
>   
> +	/* We may have lost the race and shutdown after pre_reset() */
>   	if (devinfo->shutdown)
> -		return 0;
> +		goto skip;
>   
>   	err = uas_configure_endpoints(devinfo);
>   	if (err && err != ENODEV)
> @@ -1062,6 +1063,7 @@ static int uas_post_reset(struct usb_interface *intf)
>   	scsi_report_bus_reset(shost, 0);
>   	spin_unlock_irqrestore(shost->host_lock, flags);
>   
> +skip:
>   	scsi_unblock_requests(shost);
>   
>   	return err ? 1 : 0;
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html