* [RFC]fix races in and improve CDC-WDM
@ 2020-09-21 11:20 Oliver Neukum
2020-09-21 11:20 ` [RFC 1/8] CDC-WDM: fix hangs in flush() Oliver Neukum
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb
Tests in multithreaded environments and a thorough review have
shown hangs and other issues in the driver
This version introduces nicer locking
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC 1/8] CDC-WDM: fix hangs in flush()
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 2/8] CDC-WDM: introduce a timeout " Oliver Neukum
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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] 9+ messages in thread
* [RFC 2/8] CDC-WDM: introduce a timeout in flush()
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
2020-09-21 11:20 ` [RFC 1/8] CDC-WDM: fix hangs in flush() Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 3/8] CDC-WDM: making flush() interruptible Oliver Neukum
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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] 9+ messages in thread
* [RFC 3/8] CDC-WDM: making flush() interruptible
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
2020-09-21 11:20 ` [RFC 1/8] CDC-WDM: fix hangs in flush() Oliver Neukum
2020-09-21 11:20 ` [RFC 2/8] CDC-WDM: introduce a timeout " Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 4/8] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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 -EINTR. 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] 9+ messages in thread
* [RFC 4/8] CDC-WDM: remove use of intf->dev after potential disconnect
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
` (2 preceding siblings ...)
2020-09-21 11:20 ` [RFC 3/8] CDC-WDM: making flush() interruptible Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 5/8] CDC-WDM: fix race reporting errors in flush Oliver Neukum
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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 | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index eee19532e67e..40661b9f6775 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)
@@ -611,9 +615,7 @@ static int wdm_flush(struct file *file, fl_owner_t id)
if (rv < 0)
return -EINTR;
rv = desc->werr;
- if (rv < 0)
- dev_err(&desc->intf->dev, "Error in flush path: %d\n",
- rv);
+ desc->werr = 0;
return usb_translate_errors(rv);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC 5/8] CDC-WDM: fix race reporting errors in flush
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
` (3 preceding siblings ...)
2020-09-21 11:20 ` [RFC 4/8] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 6/8] CDC-WDM: correct error reporting in write() Oliver Neukum
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 40661b9f6775..89929f6438e3 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -614,8 +614,11 @@ 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);
return usb_translate_errors(rv);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC 6/8] CDC-WDM: correct error reporting in write()
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
` (4 preceding siblings ...)
2020-09-21 11:20 ` [RFC 5/8] CDC-WDM: fix race reporting errors in flush Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 7/8] CDC-WDM: implement fsync Oliver Neukum
2020-09-21 11:20 ` [RFC 8/8] CDC-WDM: reduce scope of wdm_mutex Oliver Neukum
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 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] 9+ messages in thread
* [RFC 7/8] CDC-WDM: implement fsync
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
` (5 preceding siblings ...)
2020-09-21 11:20 ` [RFC 6/8] CDC-WDM: correct error reporting in write() Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
2020-09-21 11:20 ` [RFC 8/8] CDC-WDM: reduce scope of wdm_mutex Oliver Neukum
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
Some users want to be very sure that data has gone out to the
device. This needs fsync.
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 | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index f952eec87b0f..7607ab2bbe07 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -590,6 +590,33 @@ static ssize_t wdm_read
return rv;
}
+/*
+ * The difference to flush is that we wait forever. If you don't like
+ * that behavior, you need to send a signal.
+ */
+
+static int wdm_fsync(struct file *file, loff_t start, loff_t end, int datasync)
+{
+ struct wdm_device *desc = file->private_data;
+ int rv;
+
+ rv = wait_event_interruptible(desc->wait,
+ !test_bit(WDM_IN_USE, &desc->flags) ||
+ test_bit(WDM_DISCONNECTING, &desc->flags));
+
+ if (test_bit(WDM_DISCONNECTING, &desc->flags))
+ return -ENODEV;
+ if (rv < 0)
+ return -EINTR;
+
+ spin_lock_irq(&desc->iuspin);
+ rv = desc->werr;
+ desc->werr = 0;
+ spin_unlock_irq(&desc->iuspin);
+
+ return usb_translate_errors(rv);
+}
+
static int wdm_flush(struct file *file, fl_owner_t id)
{
struct wdm_device *desc = file->private_data;
@@ -750,6 +777,7 @@ static const struct file_operations wdm_fops = {
.owner = THIS_MODULE,
.read = wdm_read,
.write = wdm_write,
+ .fsync = wdm_fsync,
.open = wdm_open,
.flush = wdm_flush,
.release = wdm_release,
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC 8/8] CDC-WDM: reduce scope of wdm_mutex
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
` (6 preceding siblings ...)
2020-09-21 11:20 ` [RFC 7/8] CDC-WDM: implement fsync Oliver Neukum
@ 2020-09-21 11:20 ` Oliver Neukum
7 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2020-09-21 11:20 UTC (permalink / raw)
To: penguin-kernel, bjorn, linux-usb; +Cc: Oliver Neukum
Use the global mutex for as short as possible in open()
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-wdm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 7607ab2bbe07..230fe66828e3 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -704,6 +704,7 @@ static int wdm_open(struct inode *inode, struct file *file)
/* using write lock to protect desc->count */
mutex_lock(&desc->wlock);
+ mutex_unlock(&wdm_mutex);
if (!desc->count++) {
/* in case flush() had timed out */
usb_kill_urb(desc->command);
@@ -716,13 +717,14 @@ static int wdm_open(struct inode *inode, struct file *file)
"Error submitting int urb - %d\n", rv);
rv = usb_translate_errors(rv);
}
+ if (desc->count == 1)
+ desc->manage_power(intf, 1);
} else {
rv = 0;
}
mutex_unlock(&desc->wlock);
- if (desc->count == 1)
- desc->manage_power(intf, 1);
usb_autopm_put_interface(desc->intf);
+ return rv;
out:
mutex_unlock(&wdm_mutex);
return rv;
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-21 11:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21 11:20 [RFC]fix races in and improve CDC-WDM Oliver Neukum
2020-09-21 11:20 ` [RFC 1/8] CDC-WDM: fix hangs in flush() Oliver Neukum
2020-09-21 11:20 ` [RFC 2/8] CDC-WDM: introduce a timeout " Oliver Neukum
2020-09-21 11:20 ` [RFC 3/8] CDC-WDM: making flush() interruptible Oliver Neukum
2020-09-21 11:20 ` [RFC 4/8] CDC-WDM: remove use of intf->dev after potential disconnect Oliver Neukum
2020-09-21 11:20 ` [RFC 5/8] CDC-WDM: fix race reporting errors in flush Oliver Neukum
2020-09-21 11:20 ` [RFC 6/8] CDC-WDM: correct error reporting in write() Oliver Neukum
2020-09-21 11:20 ` [RFC 7/8] CDC-WDM: implement fsync Oliver Neukum
2020-09-21 11:20 ` [RFC 8/8] CDC-WDM: reduce scope of wdm_mutex 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.