* [RFC 1/6] CDC-WDM: fix hangs in flush()
@ 2020-09-10 12:21 Oliver Neukum
2020-09-10 12:21 ` [RFC 2/6] CDC-WDM: introduce a timeout " Oliver Neukum
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
In a multithreaded scenario 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, while a flush may be waiting for an intended but
not started Io.
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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index e3db6fbeadef..adb3fc307083 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)
@@ -424,6 +424,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,6 +587,7 @@ 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,
/*
@@ -600,11 +602,12 @@ static int wdm_flush(struct file *file, fl_owner_t id)
/* cannot dereference desc->intf if WDM_DISCONNECTING */
if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
- if (desc->werr < 0)
+ rv = desc->werr;
+ if (rv < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
- desc->werr);
+ rv);
- return usb_translate_errors(desc->werr);
+ return usb_translate_errors(rv);
}
static __poll_t wdm_poll(struct file *file, struct poll_table_struct *wait)
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC 2/6] CDC-WDM: introduce a timeout in flush()
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
@ 2020-09-10 12:21 ` Oliver Neukum
2020-09-10 12:21 ` [PATCH 3/6] CDC-WDM: making flush() interruptible Oliver Neukum
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
Malicious or defective hardware may cease communication while
flush() is running. In last consequence we need a timeout,
as hardware that remains silent must be ignored.
The 30 seconds are coming out of thin air.
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 | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index adb3fc307083..fef011dc8dc4 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() is 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
@@ -589,7 +592,7 @@ 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_timeout(desc->wait,
/*
* needs both flags. We cannot do with one
* because resetting it would cause a race
@@ -597,11 +600,15 @@ 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;
+
rv = desc->werr;
if (rv < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
@@ -662,6 +669,8 @@ static int wdm_open(struct inode *inode, struct file *file)
/* using write lock to protect desc->count */
mutex_lock(&desc->wlock);
if (!desc->count++) {
+ /* in case flush() had timed out */
+ usb_kill_urb(desc->command);
desc->werr = 0;
desc->rerr = 0;
rv = usb_submit_urb(desc->validity, GFP_KERNEL);
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/6] CDC-WDM: making flush() interruptible
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
2020-09-10 12:21 ` [RFC 2/6] CDC-WDM: introduce a timeout " Oliver Neukum
@ 2020-09-10 12:21 ` Oliver Neukum
2020-09-10 12:21 ` [RFC 4/6] CDC-WDM: fix race reporting errors in flush Oliver Neukum
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
There is no need for flush() to be uninterruptible. close(2)
is allowed to return -EAGAIN. Change it.
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 | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index fef011dc8dc4..eee19532e67e 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -592,7 +592,7 @@ static int wdm_flush(struct file *file, fl_owner_t id)
struct wdm_device *desc = file->private_data;
int rv;
- rv = wait_event_timeout(desc->wait,
+ rv = wait_event_interruptible_timeout(desc->wait,
/*
* needs both flags. We cannot do with one
* because resetting it would cause a race
@@ -608,7 +608,8 @@ static int wdm_flush(struct file *file, fl_owner_t id)
return -ENODEV;
if (!rv)
return -EIO;
-
+ if (rv < 0)
+ return -EINTR;
rv = desc->werr;
if (rv < 0)
dev_err(&desc->intf->dev, "Error in flush path: %d\n",
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC 4/6] CDC-WDM: fix race reporting errors in flush
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
2020-09-10 12:21 ` [RFC 2/6] CDC-WDM: introduce a timeout " Oliver Neukum
2020-09-10 12:21 ` [PATCH 3/6] CDC-WDM: making flush() interruptible Oliver Neukum
@ 2020-09-10 12:21 ` Oliver Neukum
2020-09-10 12:21 ` [RFC 5/6] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
2020-09-10 12:21 ` [RFC 6/6] CDC-WDM: correct error reporting in write() Oliver Neukum
4 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
In case a race was lost and multiple fds used,
an error could be reported multiple times. To fix
this a spinlock must be taken.
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-wdm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index eee19532e67e..930000b1d43b 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -610,10 +610,14 @@ static int wdm_flush(struct file *file, fl_owner_t id)
return -EIO;
if (rv < 0)
return -EINTR;
+
+ spin_lock_irq(&desc->iuspin);
rv = desc->werr;
+ desc->werr = 0;
+ spin_unlock_irq(&desc->iuspin);
+
if (rv < 0)
- dev_err(&desc->intf->dev, "Error in flush path: %d\n",
- rv);
+ dev_err(&desc->intf->dev, "Error in flush path: %d\n", rv);
return usb_translate_errors(rv);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC 5/6] CDC-WDM: remove use of intf->dev after potential disconnect
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
` (2 preceding siblings ...)
2020-09-10 12:21 ` [RFC 4/6] CDC-WDM: fix race reporting errors in flush Oliver Neukum
@ 2020-09-10 12:21 ` Oliver Neukum
2020-09-10 12:21 ` [RFC 6/6] CDC-WDM: correct error reporting in write() Oliver Neukum
4 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
After a disconnect intf->dev is not a valid pointer any longer
as flush() uses it only for logging purposes logging is not
worth it. Remove the dev_err()
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 | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 930000b1d43b..89929f6438e3 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -603,7 +603,11 @@ static int wdm_flush(struct file *file, fl_owner_t id)
test_bit(WDM_DISCONNECTING, &desc->flags),
WDM_FLUSH_TIMEOUT);
- /* cannot dereference desc->intf if WDM_DISCONNECTING */
+ /*
+ * to report the correct error.
+ * This is best effort
+ * We are inevitably racing with the hardware.
+ */
if (test_bit(WDM_DISCONNECTING, &desc->flags))
return -ENODEV;
if (!rv)
@@ -616,9 +620,6 @@ static int wdm_flush(struct file *file, fl_owner_t id)
desc->werr = 0;
spin_unlock_irq(&desc->iuspin);
- if (rv < 0)
- dev_err(&desc->intf->dev, "Error in flush path: %d\n", rv);
-
return usb_translate_errors(rv);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC 6/6] CDC-WDM: correct error reporting in write()
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
` (3 preceding siblings ...)
2020-09-10 12:21 ` [RFC 5/6] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
@ 2020-09-10 12:21 ` Oliver Neukum
4 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2020-09-10 12:21 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
In case our wait was interrupted by a disconnect, we should report
that.
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-wdm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 89929f6438e3..f952eec87b0f 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -396,6 +396,9 @@ static ssize_t wdm_write
if (test_bit(WDM_RESETTING, &desc->flags))
r = -EIO;
+ if (test_bit(WDM_DISCONNECTING, &desc->flags))
+ r = -ENODEV;
+
if (r < 0) {
rv = r;
goto out_free_mem_pm;
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-10 21:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 12:21 [RFC 1/6] CDC-WDM: fix hangs in flush() Oliver Neukum
2020-09-10 12:21 ` [RFC 2/6] CDC-WDM: introduce a timeout " Oliver Neukum
2020-09-10 12:21 ` [PATCH 3/6] CDC-WDM: making flush() interruptible Oliver Neukum
2020-09-10 12:21 ` [RFC 4/6] CDC-WDM: fix race reporting errors in flush Oliver Neukum
2020-09-10 12:21 ` [RFC 5/6] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
2020-09-10 12:21 ` [RFC 6/6] CDC-WDM: correct error reporting in write() Oliver Neukum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.