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

[Oliver Neukum <oneukum@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

Am Samstag, den 30.05.2020, 13:58 +0900 schrieb Tetsuo Handa:

Hi,

sorry for taking this long. There has been a family emergency.

> > The kernel most definitely does need to protect itself against 
> > misbehaving hardware.  Let's just leave it at that.  If you don't 
> > believe me, ask Greg KH.
> 
> I've made many locations killable (in order to reduce damage caused by OOM
> condition). But I can't make locations killable where handling SIGKILL case is
> too difficult to implement.

We can make flush interruptible. But that will not do the job. We would
get a file that cannot be closed.

> "struct file_operations"->flush() is called from filp_close() when there is
> something which has to be done before "struct file_operations"->release() is
> called.

Yes, in particular error reporting. Without flush() there is no way to
know whether the last write() has actually worked.

> As far as I read this thread, what you are trying to do sounds like allow
> "not waiting for completion of wdm_out_callback()" with only
> 's/wait_event/wait_event_intrruptible/' in wdm_flush(). Then, please do remove
> wdm_flush() call itself.

That would break error reporting. That flush() waits for IO to complete
is basically a side effect. You can know whether IO has worked after it
is finished.

> Therefore, again, please show me as a patch first.

Sure, attached. The difficulty here is that I see three possible
interacting errors, two of which are races.

	Regards
		Oliver

[-- Attachment #2: 0001-CDC-WDM-fix-hangs-in-flush.patch --]
[-- Type: text/x-patch, Size: 3133 bytes --]

From 27cd2e25b37af973b61b77217fa2dad822889ff8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 24 Jun 2020 10:52:03 +0200
Subject: [PATCH] CDC-WDM: fix hangs in flush()

When flushing a task needs to wait a bounded time, as a hardware failure
could mean eternal sleep. So an arbitrary timeout is introduced.
Simply making the syscall interruptible will not do the job,
as while the syscall would not hang, the fd would be unclosable.

In addition a flush() and a write() may be waiting for the same
IO to complete. Hence completion of output must use wake_up_all(),
even in error handling.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/class/cdc-wdm.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index e3db6fbeadef..ec5412773c57 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -58,6 +58,9 @@ MODULE_DEVICE_TABLE (usb, wdm_ids);
 
 #define WDM_MAX			16
 
+/* flush() needs to be uninterruptible, but we cannot wait forever */
+#define WDM_FLUSH_TIMEOUT	(30 * HZ)
+
 /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */
 #define WDM_DEFAULT_BUFSIZE	256
 
@@ -151,7 +154,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)
@@ -424,6 +427,7 @@ static ssize_t wdm_write
 	if (rv < 0) {
 		desc->outbuf = NULL;
 		clear_bit(WDM_IN_USE, &desc->flags);
+		wake_up_all(&desc->wait); /* for flush() */
 		dev_err(&desc->intf->dev, "Tx URB error: %d\n", rv);
 		rv = usb_translate_errors(rv);
 		goto out_free_mem_pm;
@@ -586,8 +590,9 @@ static ssize_t wdm_read
 static int wdm_flush(struct file *file, fl_owner_t id)
 {
 	struct wdm_device *desc = file->private_data;
+	int rv;
 
-	wait_event(desc->wait,
+	rv = wait_event_interruptible_timeout(desc->wait,
 			/*
 			 * needs both flags. We cannot do with one
 			 * because resetting it would cause a race
@@ -595,11 +600,16 @@ static int wdm_flush(struct file *file, fl_owner_t id)
 			 * a disconnect
 			 */
 			!test_bit(WDM_IN_USE, &desc->flags) ||
-			test_bit(WDM_DISCONNECTING, &desc->flags));
+			test_bit(WDM_DISCONNECTING, &desc->flags),
+			WDM_FLUSH_TIMEOUT);
 
 	/* cannot dereference desc->intf if WDM_DISCONNECTING */
 	if (test_bit(WDM_DISCONNECTING, &desc->flags))
 		return -ENODEV;
+	if (!rv)
+		return -EIO;
+	if (rv < 0)
+		return -EINTR;
 	if (desc->werr < 0)
 		dev_err(&desc->intf->dev, "Error in flush path: %d\n",
 			desc->werr);
@@ -656,6 +666,14 @@ static int wdm_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 
+	/*
+	 * in case flush() had timed out
+	 */
+	usb_kill_urb(desc->command);
+	spin_lock_irq(&desc->iuspin);
+	desc->werr = 0;
+	spin_unlock_irq(&desc->iuspin);
+
 	/* using write lock to protect desc->count */
 	mutex_lock(&desc->wlock);
 	if (!desc->count++) {
-- 
2.16.4