Re: [PATCH] USB: cdc-wdm: Call wake_up_all() when clearing WDM_IN_USE bit.

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Andrey Konovalov <andreyknvl@google.com>,
	Oliver Neukum <oneukum@suse.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Colin Ian King <colin.king@canonical.com>,
	Arnd Bergmann <arnd@arndb.de>,
	USB list <linux-usb@vger.kernel.org>,
	syzbot <syzbot+854768b99f19e89d7f81@syzkaller.appspotmail.com>,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Subject: Re: [PATCH] USB: cdc-wdm: Call wake_up_all() when clearing WDM_IN_USE bit.
Date: Thu, 2 Jul 2020 14:44:35 +0900	[thread overview]
Message-ID: <b347b882-a986-24c6-2b37-0b1a092931b9@i-love.sakura.ne.jp> (raw)
In-Reply-To: <c85331fc-874c-6e46-a77f-0ef1dc075308@i-love.sakura.ne.jp>

On 2020/06/23 20:20, Tetsuo Handa wrote:
> Also, if wait_event() in wdm_flush() might fail to wake up (due to close() dependency
> problem this crash report is focusing on), wait_event_interruptible() in wdm_write() might
> also fail to wake up (unless interrupted) due to the same dependency. Then, why can't we
> wait for completion of wdm_out_callback() (with reasonable timeout) inside wdm_write() ?
> 
> I feel that wdm_flush() is so bogus (which could/should be removed).
> 

I'd like to simplify like below.

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index e3db6fbeadef..9631d1054799 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -151,7 +151,7 @@ static void wdm_out_callback(struct urb *urb)
 	kfree(desc->outbuf);
 	desc->outbuf = NULL;
 	clear_bit(WDM_IN_USE, &desc->flags);
-	wake_up(&desc->wait);
+	wake_up_all(&desc->wait);
 }
 
 static void wdm_in_callback(struct urb *urb)
@@ -426,6 +426,7 @@ static ssize_t wdm_write
 		clear_bit(WDM_IN_USE, &desc->flags);
 		dev_err(&desc->intf->dev, "Tx URB error: %d\n", rv);
 		rv = usb_translate_errors(rv);
+		wake_up_all(&desc->wait);
 		goto out_free_mem_pm;
 	} else {
 		dev_dbg(&desc->intf->dev, "Tx URB has been submitted index=%d\n",
@@ -434,6 +435,24 @@ static ssize_t wdm_write
 
 	usb_autopm_put_interface(desc->intf);
 	mutex_unlock(&desc->wlock);
+	if (rv >= 0 &&
+	    /*
+	     * needs both flags. We cannot do with one
+	     * because resetting it would cause a race
+	     * with write() yet we need to signal
+	     * a disconnect
+	     */
+	    wait_event_killable_timeout(desc->wait,
+					!test_bit(WDM_IN_USE, &desc->flags) ||
+					test_bit(WDM_DISCONNECTING, &desc->flags), 30 * HZ) == 0) {
+		if (mutex_lock_killable(&desc->wlock) == 0) {
+			if (!test_bit(WDM_DISCONNECTING, &desc->flags))
+				dev_err(&desc->intf->dev,
+					"Tx URB not responding index=%d\n",
+					le16_to_cpu(req->wIndex));
+			mutex_unlock(&desc->wlock);
+		}
+	}
 	return count;
 
 out_free_mem_pm:
@@ -583,30 +602,6 @@ static ssize_t wdm_read
 	return rv;
 }
 
-static int wdm_flush(struct file *file, fl_owner_t id)
-{
-	struct wdm_device *desc = file->private_data;
-
-	wait_event(desc->wait,
-			/*
-			 * needs both flags. We cannot do with one
-			 * because resetting it would cause a race
-			 * with write() yet we need to signal
-			 * a disconnect
-			 */
-			!test_bit(WDM_IN_USE, &desc->flags) ||
-			test_bit(WDM_DISCONNECTING, &desc->flags));
-
-	/* cannot dereference desc->intf if WDM_DISCONNECTING */
-	if (test_bit(WDM_DISCONNECTING, &desc->flags))
-		return -ENODEV;
-	if (desc->werr < 0)
-		dev_err(&desc->intf->dev, "Error in flush path: %d\n",
-			desc->werr);
-
-	return usb_translate_errors(desc->werr);
-}
-
 static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
 {
 	struct wdm_device *desc = file->private_data;
@@ -730,7 +725,6 @@ static const struct file_operations wdm_fops = {
 	.read =		wdm_read,
 	.write =	wdm_write,
 	.open =		wdm_open,
-	.flush =	wdm_flush,
 	.release =	wdm_release,
 	.poll =		wdm_poll,
 	.unlocked_ioctl = wdm_ioctl,

