[RFC 0/5] fix races in CDC-WDM

[RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

CDC-WDM was not written with multithreaded users or
uncooperative devices in mind.
This leads to race conditions and hangs in flush(). 

[RFC 1/5] CDC-WDM: fix hangs in flush()

[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 7f5de956a2fc..a834294ce4e5 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

[RFC 2/5] CDC-WDM: introduce a timeout in flush()

[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 | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a834294ce4e5..329f8d5f7c8d 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,14 @@ 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",
@@ -659,6 +665,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

[RFC 3/5] CDC-WDM: making flush() interruptible

[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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 329f8d5f7c8d..f442f34f38da 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,6 +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

[RFC 4/5] CDC-WDM: fix race reporting errors in flush

[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 f442f34f38da..6071c9044574 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

[RFC 5/5] CDC-WDM: remove use of intf->dev after potential disconnect

[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 | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 6071c9044574..2dcf1fb16f2b 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -616,9 +616,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

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/08/12 22:20, Oliver Neukum wrote:
> CDC-WDM was not written with multithreaded users or
> uncooperative devices in mind.
> This leads to race conditions and hangs in flush(). 
> 

In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", since multiple users can
share "desc", wouldn't someone's usb_submit_urb() from wdm_write() gets unexpectedly
interfered by someone else's usb_kill_urb(desc->command) from wdm_open() ?

In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", don't we need to similarly
apply timeout to wait_event_interruptible() in wdm_write(), for the same problem will
happen if hardware remained silent forever?

In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
 from close(). But I think that returning -EINTR from close() is not recommended because
it can confuse multithreaded application (retrying close() upon -EINTR is not safe).

In patch "[RFC 5/5] CDC-WDM: remove use of intf->dev after potential disconnect",

        /* cannot dereference desc->intf if WDM_DISCONNECTING */
        if (test_bit(WDM_DISCONNECTING, &desc->flags))
                return -ENODEV;

can be also removed, for this check is meant for not to dereference desc->intf
after disconnect ?

Guessing from symmetry, do we need to check WDM_DISCONNECTING or WDM_RESETTING
in wait_event_interruptible_timeout() from wdm_flush() when wait_event_interruptible()
in wdm_write() is not checking WDM_DISCONNECTING nor WDM_RESETTING ?

Does it make sense to wait for response of someone else's usb_submit_urb() when
someone is calling close(), for there is no guarantee that failure notice received
via wdm_flush() via some file descriptor corresponds to usb_submit_urb() request from
wdm_write() call from that file descriptor?

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Mittwoch, den 12.08.2020, 23:29 +0900 schrieb Tetsuo Handa:
> On 2020/08/12 22:20, Oliver Neukum wrote:
> > CDC-WDM was not written with multithreaded users or
> > uncooperative devices in mind.
> > This leads to race conditions and hangs in flush(). 


Hi,

thank you for waiting. Family emergency.

> In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", since multiple users can
> share "desc", wouldn't someone's usb_submit_urb() from wdm_write() gets unexpectedly
> interfered by someone else's usb_kill_urb(desc->command) from wdm_open() ?

Yes. Fixed.

> In patch "[RFC 2/5] CDC-WDM: introduce a timeout in flush()", don't we need to similarly
> apply timeout to wait_event_interruptible() in wdm_write(), for the same problem will
> happen if hardware remained silent forever?

Technically a device may take forever. Interrupting a wait in write()
is not problematic.

> In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
>  from close(). But I think that returning -EINTR from close() is not recommended because
> it can confuse multithreaded application (retrying close() upon -EINTR is not safe).

Well, but what is the alternative? Should we ignore signals?

> In patch "[RFC 5/5] CDC-WDM: remove use of intf->dev after potential disconnect",
> 
>         /* cannot dereference desc->intf if WDM_DISCONNECTING */
>         if (test_bit(WDM_DISCONNECTING, &desc->flags))
>                 return -ENODEV;
> 
> can be also removed, for this check is meant for not to dereference desc->intf
> after disconnect ?

It can be removed, but that would make error reporting worse.

> Guessing from symmetry, do we need to check WDM_DISCONNECTING or WDM_RESETTING
> in wait_event_interruptible_timeout() from wdm_flush() when wait_event_interruptible()
> in wdm_write() is not checking WDM_DISCONNECTING nor WDM_RESETTING ?

Added

> Does it make sense to wait for response of someone else's usb_submit_urb() when
> someone is calling close(), for there is no guarantee that failure notice received
> via wdm_flush() via some file descriptor corresponds to usb_submit_urb() request from
> wdm_write() call from that file descriptor?

Well, user space may do multithreading. Whether it makes sense is
another question. We just need to return results confirming to the
standards. You noticed bugs. I think the next version will fix them.

What else would we do?

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/10 18:09, Oliver Neukum wrote:
>> Does it make sense to wait for response of someone else's usb_submit_urb() when
>> someone is calling close(), for there is no guarantee that failure notice received
>> via wdm_flush() via some file descriptor corresponds to usb_submit_urb() request from
>> wdm_write() call from that file descriptor?
> 
> Well, user space may do multithreading. Whether it makes sense is
> another question. We just need to return results confirming to the
> standards. You noticed bugs. I think the next version will fix them.

My question is how do you handle if App1 and App2 (not multithreading but
multiprocessing) shares the "desc" ? Unless

>> In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
>>  from close(). But I think that returning -EINTR from close() is not recommended because
>> it can confuse multithreaded application (retrying close() upon -EINTR is not safe).
> 
> Well, but what is the alternative? Should we ignore signals?
> 

we return the error from write() request (i.e. give up trying to report errors from
close() event), we can't pass results to the intended recipients.

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Donnerstag, den 10.09.2020, 19:01 +0900 schrieb Tetsuo Handa:
> On 2020/09/10 18:09, Oliver Neukum wrote:
> > > Does it make sense to wait for response of someone else's usb_submit_urb() when
> > > someone is calling close(), for there is no guarantee that failure notice received
> > > via wdm_flush() via some file descriptor corresponds to usb_submit_urb() request from
> > > wdm_write() call from that file descriptor?
> > 
> > Well, user space may do multithreading. Whether it makes sense is
> > another question. We just need to return results confirming to the
> > standards. You noticed bugs. I think the next version will fix them.
> 
> My question is how do you handle if App1 and App2 (not multithreading but
> multiprocessing) shares the "desc" ? Unless

Well, device locking is a function of user space. This is Unix. If you
have two uncoordinated writers to a device, you cannot expect sensible
results. You can expect that the driver does not hang, of course.

> > > In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
> > >  from close(). But I think that returning -EINTR from close() is not recommended because
> > > it can confuse multithreaded application (retrying close() upon -EINTR is not safe).
> > 
> > Well, but what is the alternative? Should we ignore signals?
> > 
> 
> we return the error from write() request (i.e. give up trying to report errors from
> close() event), we can't pass results to the intended recipients.

That means

* harming the single threaded for the sake of the few multithreaded
* it would not work for O_NONBLOCK
* if you use a device from multiple threads or tasks, locking is your
problem

Is there something we can do in flush()?

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/15 18:14, Oliver Neukum wrote:
>>>> In patch "[RFC 3/5] CDC-WDM: making flush() interruptible", it is legal to return -EINTR
>>>>  from close(). But I think that returning -EINTR from close() is not recommended because
>>>> it can confuse multithreaded application (retrying close() upon -EINTR is not safe).
>>>
>>> Well, but what is the alternative? Should we ignore signals?
>>>
>>
>> we return the error from write() request (i.e. give up trying to report errors from
>> close() event), we can't pass results to the intended recipients.
> 
> That means
> 
> * harming the single threaded for the sake of the few multithreaded

What is wrong with the single threaded user? As I describe below, there is no guarantee
that wdm_write() can complete immediately (even if O_NONBLOCK flag is set).

> * it would not work for O_NONBLOCK

It does work for O_NONBLOCK. Please see my proposal at
https://lkml.kernel.org/r/b347b882-a986-24c6-2b37-0b1a092931b9@i-love.sakura.ne.jp .
Since my proposal calls mutex_unlock() before start waiting for response, my
proposal does not block mutex_lock_interruptible() from O_NONBLOCK write().

O_NONBLOCK cannot guarantee that "we do not wait for memory allocation request by
memdup_user()/copy_from_user()/usb_submit_urb(GFP_KERNEL)". It is possible that
O_NONBLOCK write() is blocked for many _minutes_ at (at least) these three locations.

O_NONBLOCK only guarantees that "it does not wait when we can't start immediately".

> * if you use a device from multiple threads or tasks, locking is your
> problem

You mean "a device" as "struct wdm_device *desc" ? Then, it does not matter because
the mutex, buffer, status etc. are per "struct wdm_device *desc" objects. Nobody will
be disturbed by somebody else using different "struct wdm_device *desc".

> 
> Is there something we can do in flush()?

I consider that wdm_flush() is a wrong place to return an error. It is possible that
a userspace process reaches wdm_flush() due to being killed by SIGKILL (rather than
via calling close() syscall). Then, that userspace process will never receive the error
fetched from wdm_flush(). Also, if that userspace process is killed by the OOM killer,
being able to terminate and release resources as soon as possible is more preferable
than try to wait for response.

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Dienstag, den 15.09.2020, 19:30 +0900 schrieb Tetsuo Handa:
> On 2020/09/15 18:14, Oliver Neukum wrote

> > Is there something we can do in flush()?
> 
> I consider that wdm_flush() is a wrong place to return an error. It is possible that

I am afraid that is a basic problem we need to resolve. As I understand
 it, flush() as a method exists precisely to report errors. Otherwise
you could implement it in release(). But this is not called for every
close().

Hence a driver is supposed to start IO upon write() and report the
result to the next call, which can be either write() or close(), the
latter corresponding to flush().

> a userspace process reaches wdm_flush() due to being killed by SIGKILL (rather than
> via calling close() syscall). Then, that userspace process will never receive the error

If you are killed by a signal you are in a race condition
anyway. It cannot be handled.

> fetched from wdm_flush(). Also, if that userspace process is killed by the OOM killer,
> being able to terminate and release resources as soon as possible is more preferable
> than try to wait for response.

Right, so should we just proceed in case of a dieing task? How do we
do that?

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/16 19:18, Oliver Neukum wrote:
> Am Dienstag, den 15.09.2020, 19:30 +0900 schrieb Tetsuo Handa:
>> On 2020/09/15 18:14, Oliver Neukum wrote
> 
>>> Is there something we can do in flush()?
>>
>> I consider that wdm_flush() is a wrong place to return an error. It is possible that
> 
> I am afraid that is a basic problem we need to resolve. As I understand
>  it, flush() as a method exists precisely to report errors. Otherwise
> you could implement it in release(). But this is not called for every
> close().

I think fsync() or ioctl() is a better method for reporting errors.

But anyway I consider that any error from N'th write() request should be
returned by N'th write() request.

> 
> Hence a driver is supposed to start IO upon write() and report the
> result to the next call, which can be either write() or close(), the
> latter corresponding to flush().

Whether N'th write() request succeeded remains unknown until (N+1)'th
write() request or close() request is issued? That sounds a strange design.

If there is nothing more to write(), how userspace process knows whether
N'th write() request succeeded? Wait for writability using poll() ?

How do we guarantee that N'th write() request already set desc->werr before
(N+1)'th next write() request is issued? If (N+1)'th write() request reached
memdup_user() before desc->werr is set by callback of N'th write() request,
(N+1)'th write() request will fail to report the error from N'th write() request.
Yes, that error would be reported by (N+2)'th write() request, but the userspace
process might have already discarded data needed for taking some actions (e.g.
print error messages, retry the write() request with same argument).

> 
>> a userspace process reaches wdm_flush() due to being killed by SIGKILL (rather than
>> via calling close() syscall). Then, that userspace process will never receive the error
> 
> If you are killed by a signal you are in a race condition
> anyway. It cannot be handled.

What is the purpose of sending the error to the userspace process via write() or close()?
Isn't the purpose to allow userspace process to do something (e.g. print error messages,
retry the write() request with same argument) ? If close() returned an error, it might be
too late to retry the write() request with same argument.

> 
>> fetched from wdm_flush(). Also, if that userspace process is killed by the OOM killer,
>> being able to terminate and release resources as soon as possible is more preferable
>> than try to wait for response.
> 
> Right, so should we just proceed in case of a dieing task? How do we
> do that?

We need to avoid waiting at wdm_flush() if killed.

And I think that wdm_flush() is a strange interface for reporting the error.

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Mittwoch, den 16.09.2020, 20:14 +0900 schrieb Tetsuo Handa:
> On 2020/09/16 19:18, Oliver Neukum wrote:
> > Am Dienstag, den 15.09.2020, 19:30 +0900 schrieb Tetsuo Handa:
> > > On 2020/09/15 18:14, Oliver Neukum wrote
> > > > Is there something we can do in flush()?
> > > 
> > > I consider that wdm_flush() is a wrong place to return an error. It is possible that
> > 
> > I am afraid that is a basic problem we need to resolve. As I understand
> >  it, flush() as a method exists precisely to report errors. Otherwise
> > you could implement it in release(). But this is not called for every
> > close().
> 
> I think fsync() or ioctl() is a better method for reporting errors.

Very well, I am implementing fsync(). However, I must say that the very
existance of fsync() tells us that write() is not expected to push
data out to devices and hence report results.

> Whether N'th write() request succeeded remains unknown until (N+1)'th
> write() request or close() request is issued? That sounds a strange design.

Welcome to the world of Unix. This is necessary if you want to block
as rarely as possible.

> If there is nothing more to write(), how userspace process knows whether
> N'th write() request succeeded? Wait for writability using poll() ?

Technically fsync(). poll() will tell you that you can write again.
That is a related but different concept.

> What is the purpose of sending the error to the userspace process via write() or close()?

Yes. However to do so, user space must be running. That the death
of a process interferes with error handling is independent from that.

> And I think that wdm_flush() is a strange interface for reporting the error.

Well, POSIX is what it is.The close() syscall is supposed to return
errors. Hence flush() must report them.

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/17 18:50, Oliver Neukum wrote:
> Am Mittwoch, den 16.09.2020, 20:14 +0900 schrieb Tetsuo Handa:
>> On 2020/09/16 19:18, Oliver Neukum wrote:
>>> Am Dienstag, den 15.09.2020, 19:30 +0900 schrieb Tetsuo Handa:
>>>> On 2020/09/15 18:14, Oliver Neukum wrote
>>>>> Is there something we can do in flush()?
>>>>
>>>> I consider that wdm_flush() is a wrong place to return an error. It is possible that
>>>
>>> I am afraid that is a basic problem we need to resolve. As I understand
>>>  it, flush() as a method exists precisely to report errors. Otherwise
>>> you could implement it in release(). But this is not called for every
>>> close().
>>
>> I think fsync() or ioctl() is a better method for reporting errors.
> 
> Very well, I am implementing fsync(). However, I must say that the very
> existance of fsync() tells us that write() is not expected to push
> data out to devices and hence report results.

If you ask userspace programs to be updated to call fsync(), we can ask
userspace programs be updated to call ioctl().

I expected you to implement wdm_ioctl() for fetching last error code. Then,

> 
>> Whether N'th write() request succeeded remains unknown until (N+1)'th
>> write() request or close() request is issued? That sounds a strange design.
> 
> Welcome to the world of Unix. This is necessary if you want to block
> as rarely as possible.

we can apply my proposal (wait for response at wdm_write() by default) as a baseline
for not to break existing userspace programs (except latency), followed by a patch
which allows userspace programs to use that wdm_ioctl() in order not to wait for
response at wdm_write(), which is enabled by calling wdm_ioctl() (in order to
recover latency caused by waiting for response at wdm_write()).



>> What is the purpose of sending the error to the userspace process via write() or close()?
> 
> Yes. However to do so, user space must be running. That the death
> of a process interferes with error handling is independent from that.

Why need to send the error to the userspace process when that process was killed?
My question

  Isn't the purpose to allow userspace process to do something (e.g. print error messages,
  retry the write() request with same argument) ? If close() returned an error, it might be
  too late to retry the write() request with same argument.

which is unrelated to the userspace process being killed. My suggestion is that we can send
the error from wdm_write() (for synchronous mode) or wdm_ioctl() (for asynchronous mode).

> 
>> And I think that wdm_flush() is a strange interface for reporting the error.
> 
> Well, POSIX is what it is.The close() syscall is supposed to return
> errors. Hence flush() must report them.

If we check the error at wdm_write() or wdm_ioctl(), there is no error to report at
wdm_flush(). Therefore, we can remove wdm_flush() completely.



I can't read this series without squashing into single patch. Making changes which
will be after all removed in [RFC 5/7] is sad. Please do [RFC 5/7] before [RFC 4/7].
Then, you won't need to make unneeded modifications. I'd like to see one cleanup
patch, one possible unsafe dereference fix patch, and one deadlock avoidance patch.



You did not answer to

  How do we guarantee that N'th write() request already set desc->werr before
  (N+1)'th next write() request is issued? If (N+1)'th write() request reached
  memdup_user() before desc->werr is set by callback of N'th write() request,
  (N+1)'th write() request will fail to report the error from N'th write() request.
  Yes, that error would be reported by (N+2)'th write() request, but the userspace
  process might have already discarded data needed for taking some actions (e.g.
  print error messages, retry the write() request with same argument).

. At least I think that

        spin_lock_irq(&desc->iuspin);
        we = desc->werr;
        desc->werr = 0;
        spin_unlock_irq(&desc->iuspin);
        if (we < 0)
                return usb_translate_errors(we);

in wdm_write() should be moved to after !test_bit(WDM_IN_USE, &desc->flags).



In [RFC 2/7], I think that

                /* in case flush() had timed out */
                usb_kill_urb(desc->command);

which is called only when desc->count == 0 in wdm_open() is pointless, for since
desc->count is incremented/decremented with wdm_mutex held, kill_urbs(desc) which
is called when desc->count == 0 in wdm_release() must have already called
usb_kill_urb(desc->command) before allowing wdm_open() to reach there.

In addition, is

        /* using write lock to protect desc->count */
        mutex_lock(&desc->wlock);

required? Isn't wdm_mutex that is actually protecting desc->count from modification?
If it is desc->wlock that is actually protecting desc->count, the !desc->count check
in wdm_release() and the desc->count == 1 check in wdm_open() have to be done with
desc->wlock held.



In [RFC 3/7], patch description says

  There is no need for flush() to be uninterruptible. close(2)
  is allowed to return -EAGAIN. Change it.

but the code does

	if (rv < 0)
		return -EINTR;

. Which error code do you want to use? (I still prefer removing wdm_flush() completely...)

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Donnerstag, den 17.09.2020, 20:24 +0900 schrieb Tetsuo Handa:
> On 2020/09/17 18:50, Oliver Neukum wrote:
> > Am Mittwoch, den 16.09.2020, 20:14 +0900 schrieb Tetsuo Handa:

> If you ask userspace programs to be updated to call fsync(), we can ask
> userspace programs be updated to call ioctl().
> 
> I expected you to implement wdm_ioctl() for fetching last error code. Then,

Again, we are not redefining APIs. The APIs for character devices are
well defined by POSIX. Please see the man pages. Introducing a whole
new ioctl() is out of question.

The API and its semantics are clear. Write schedules a write:

       A  successful  return  from  write() does not make any guarantee that data has been committed to disk.  On some filesystems, including NFS, it does not even guarantee that space has successfully been reserved for the data.  In this case, some errors might be
       delayed until a future write(2), fsync(2), or even close(2).  The only way to be sure is to call fsync(2) after you are done writing all your data.

Fsync flushes data:

       fsync()  transfers ("flushes") all modified in-core data of (i.e., modified buffer cache pages for) the file referred to by the file descriptor fd to the disk device (or other permanent storage device) so that all changed information can be retrieved even if
       the system crashes or is rebooted.  This includes writing through or flushing a disk cache if present.  The call blocks until the device reports that the transfer has completed.

If user space does not call fsync(), the error is supposed to be reported
by the next write() and if there is no next write(), close() shall report it.

> we can apply my proposal (wait for response at wdm_write() by default) as a baseline
> for not to break existing userspace programs (except latency), followed by a patch
> which allows userspace programs to use that wdm_ioctl() in order not to wait for
> response at wdm_write(), which is enabled by calling wdm_ioctl() (in order to
> recover latency caused by waiting for response at wdm_write()).

I am sorry, but the API is defined by POSIX.

> > > What is the purpose of sending the error to the userspace process via write() or close()?
> > 
> > Yes. However to do so, user space must be running. That the death
> > of a process interferes with error handling is independent from that.
> 
> Why need to send the error to the userspace process when that process was killed?
> My question
> 
>   Isn't the purpose to allow userspace process to do something (e.g. print error messages,
>   retry the write() request with same argument) ? If close() returned an error, it might be
>   too late to retry the write() request with same argument.

Yes. Technically you need to use fsync(). Hence I implemented it.

> If we check the error at wdm_write() or wdm_ioctl(), there is no error to report at
> wdm_flush(). Therefore, we can remove wdm_flush() completely.

Again, the API is defined by POSIX. If user space calls write() and
then close(), close() must report the error.

> I can't read this series without squashing into single patch. Making changes which
> will be after all removed in [RFC 5/7] is sad. Please do [RFC 5/7] before [RFC 4/7].

Done.

> Then, you won't need to make unneeded modifications. I'd like to see one cleanup
> patch, one possible unsafe dereference fix patch, and one deadlock avoidance patch.

This needs to partially go into stable. Hence fixes must come first.

> You did not answer to
> 
>   How do we guarantee that N'th write() request already set desc->werr before
>   (N+1)'th next write() request is issued? If (N+1)'th write() request reached
>   memdup_user() before desc->werr is set by callback of N'th write() request,
>   (N+1)'th write() request will fail to report the error from N'th write() request.
>   Yes, that error would be reported by (N+2)'th write() request, but the userspace
>   process might have already discarded data needed for taking some actions (e.g.
>   print error messages, retry the write() request with same argument).

We don't, nor do we have to. You are right, error reporting can be
improved. I implemented fsync() to do so.

> . At least I think that
> 
>         spin_lock_irq(&desc->iuspin);
>         we = desc->werr;
>         desc->werr = 0;
>         spin_unlock_irq(&desc->iuspin);
>         if (we < 0)
>                 return usb_translate_errors(we);
> 
> in wdm_write() should be moved to after !test_bit(WDM_IN_USE, &desc->flags).

Why?

> In [RFC 2/7], I think that
> 
>                 /* in case flush() had timed out */
>                 usb_kill_urb(desc->command);
> 
> which is called only when desc->count == 0 in wdm_open() is pointless, for since
> desc->count is incremented/decremented with wdm_mutex held, kill_urbs(desc) which
> is called when desc->count == 0 in wdm_release() must have already called
> usb_kill_urb(desc->command) before allowing wdm_open() to reach there.

You are right. I am going to remove it.

> In addition, is
> 
>         /* using write lock to protect desc->count */
>         mutex_lock(&desc->wlock);
> 
> required? Isn't wdm_mutex that is actually protecting desc->count from modification?
> If it is desc->wlock that is actually protecting desc->count, the !desc->count check
> in wdm_release() and the desc->count == 1 check in wdm_open() have to be done with
> desc->wlock held.

Correct. So should wdm_mutex be dropped earlier?

> In [RFC 3/7], patch description says
> 
>   There is no need for flush() to be uninterruptible. close(2)
>   is allowed to return -EAGAIN. Change it.
> 
> but the code does
> 
> 	if (rv < 0)
> 		return -EINTR;
> 
> . Which error code do you want to use? (I still prefer removing wdm_flush() completely...)

-EINTR. Sorry. I shall change the description.

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/17 23:17, Oliver Neukum wrote:
> The API and its semantics are clear. Write schedules a write:
> 
>        A  successful  return  from  write() does not make any guarantee that data has been committed to disk.  On some filesystems, including NFS, it does not even guarantee that space has successfully been reserved for the data.  In this case, some errors might be
>        delayed until a future write(2), fsync(2), or even close(2).  The only way to be sure is to call fsync(2) after you are done writing all your data.

But I think that this leaves a room for allowing write() to imply fflush()
(i.e. write() is allowed to wait for data to be committed to disk).

> 
> Fsync flushes data:
> 
>        fsync()  transfers ("flushes") all modified in-core data of (i.e., modified buffer cache pages for) the file referred to by the file descriptor fd to the disk device (or other permanent storage device) so that all changed information can be retrieved even if
>        the system crashes or is rebooted.  This includes writing through or flushing a disk cache if present.  The call blocks until the device reports that the transfer has completed.
> 
> If user space does not call fsync(), the error is supposed to be reported
> by the next write() and if there is no next write(), close() shall report it.

Where does "the next" (and not "the next after the next") write() come from?

>> You did not answer to
>>
>>   How do we guarantee that N'th write() request already set desc->werr before
>>   (N+1)'th next write() request is issued? If (N+1)'th write() request reached
>>   memdup_user() before desc->werr is set by callback of N'th write() request,
>>   (N+1)'th write() request will fail to report the error from N'th write() request.
>>   Yes, that error would be reported by (N+2)'th write() request, but the userspace
>>   process might have already discarded data needed for taking some actions (e.g.
>>   print error messages, retry the write() request with same argument).
> 
> We don't, nor do we have to. You are right, error reporting can be
> improved. I implemented fsync() to do so.

You are saying that if user space does not call fsync(), the error is allowed to be
reported by the next after the next (in other words, (N+2)'th) write() ?

> 
>> . At least I think that
>>
>>         spin_lock_irq(&desc->iuspin);
>>         we = desc->werr;
>>         desc->werr = 0;
>>         spin_unlock_irq(&desc->iuspin);
>>         if (we < 0)
>>                 return usb_translate_errors(we);
>>
>> in wdm_write() should be moved to after !test_bit(WDM_IN_USE, &desc->flags).
> 
> Why?

Otherwise, we can't make sure (N+1)'th write() will report error from N'th write().

Since I don't know the characteristics of data passed via wdm_write() (I guess that
the data is some stateful controlling commands rather than meaningless byte stream),
I guess that (N+1)'th wdm_write() attempt should be made only after confirming that
N'th wdm_write() attempt received wdm_callback() response. To preserve state / data
used by N'th wdm_write() attempt, reporting the error from too late write() attempt
would be useless.



>> In addition, is
>>
>>         /* using write lock to protect desc->count */
>>         mutex_lock(&desc->wlock);
>>
>> required? Isn't wdm_mutex that is actually protecting desc->count from modification?
>> If it is desc->wlock that is actually protecting desc->count, the !desc->count check
>> in wdm_release() and the desc->count == 1 check in wdm_open() have to be done with
>> desc->wlock held.
> 
> Correct. So should wdm_mutex be dropped earlier?

If recover_from_urb_loss() can tolerate stale desc->count value, wdm_mutex already
protects desc->count. I don't know how this module works. I don't know whether
wdm_mutex and/or desc->wlock is held when recover_from_urb_loss() is called from
wdm_resume(). It seems that desc->wlock is held but wdm_mutex is not held when
recover_from_urb_loss() is called from wdm_post_reset().



By the way, after the fixes, we could replace

  spin_lock_irq(&desc->iuspin);
  rv = desc->werr;
  desc->werr = 0;
  spin_unlock_irq(&desc->iuspin);

with

  rv = xchg(&desc->werr, 0);

and avoid spin_lock_irq()/spin_unlock_irq() because there are many
locations which needs to check and clear the error...

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Freitag, den 18.09.2020, 01:17 +0900 schrieb Tetsuo Handa:
> On 2020/09/17 23:17, Oliver Neukum wrote:
> > The API and its semantics are clear. Write schedules a write:
> > 
> >        A  successful  return  from  write() does not make any guarantee that data has been committed to disk.  On some filesystems, including NFS, it does not even guarantee that space has successfully been reserved for the data.  In this case, some errors might be
> >        delayed until a future write(2), fsync(2), or even close(2).  The only way to be sure is to call fsync(2) after you are done writing all your data.
> 
> But I think that this leaves a room for allowing write() to imply fflush()
> (i.e. write() is allowed to wait for data to be committed to disk).

That would be inferior and very bad for the non-blocking case.

> > If user space does not call fsync(), the error is supposed to be reported
> > by the next write() and if there is no next write(), close() shall report it.
> 
> Where does "the next" (and not "the next after the next") write() come from?

We would indeed by on spec. However, we perform best if we return an
error as soon as possible.

> You are saying that if user space does not call fsync(), the error is allowed to be
> reported by the next after the next (in other words, (N+2)'th) write() ?

Yes. The man page is clear on that.

> > > . At least I think that
> > > 
> > >         spin_lock_irq(&desc->iuspin);
> > >         we = desc->werr;
> > >         desc->werr = 0;
> > >         spin_unlock_irq(&desc->iuspin);
> > >         if (we < 0)
> > >                 return usb_translate_errors(we);
> > > 
> > > in wdm_write() should be moved to after !test_bit(WDM_IN_USE, &desc->flags).
> > 
> > Why?
> 
> Otherwise, we can't make sure (N+1)'th write() will report error from N'th write().

We should move the test for reporting errors later, so that it is sure
to be carried out? I am afraid I cannot follow that logic.

> Since I don't know the characteristics of data passed via wdm_write() (I guess that
> the data is some stateful controlling commands rather than meaningless byte stream),
> I guess that (N+1)'th wdm_write() attempt should be made only after confirming that
> N'th wdm_write() attempt received wdm_callback() response. To preserve state / data
> used by N'th wdm_write() attempt, reporting the error from too late write() attempt
> would be useless.

We cannot make assumptions on how user space uses the driver. Somebody
manually connecting and typing in commands letter by letter must also
work.

We can optimize for the common case, but we must operate according to
the specs.
> 
> 
> 
> > > In addition, is
> > > 
> > >         /* using write lock to protect desc->count */
> > >         mutex_lock(&desc->wlock);
> > > 
> > > required? Isn't wdm_mutex that is actually protecting desc->count from modification?
> > > If it is desc->wlock that is actually protecting desc->count, the !desc->count check
> > > in wdm_release() and the desc->count == 1 check in wdm_open() have to be done with
> > > desc->wlock held.
> > 
> > Correct. So should wdm_mutex be dropped earlier?
> 
> If recover_from_urb_loss() can tolerate stale desc->count value, wdm_mutex already

It cannot.

> protects desc->count. I don't know how this module works. I don't know whether
> wdm_mutex and/or desc->wlock is held when recover_from_urb_loss() is called from
> wdm_resume(). It seems that desc->wlock is held but wdm_mutex is not held when
> recover_from_urb_loss() is called from wdm_post_reset().

Indeed.
> 
> 
> 
> By the way, after the fixes, we could replace
> 
>   spin_lock_irq(&desc->iuspin);
>   rv = desc->werr;
>   desc->werr = 0;
>   spin_unlock_irq(&desc->iuspin);
> 
> with
> 
>   rv = xchg(&desc->werr, 0);
> 
> and avoid spin_lock_irq()/spin_unlock_irq() because there are many
> locations which needs to check and clear the error...

Have you checked whether this has implications on memory ordering?

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/21 19:52, Oliver Neukum wrote:
>>> If user space does not call fsync(), the error is supposed to be reported
>>> by the next write() and if there is no next write(), close() shall report it.
>>
>> Where does "the next" (and not "the next after the next") write() come from?
> 
> We would indeed by on spec. However, we perform best if we return an
> error as soon as possible.
> 
>> You are saying that if user space does not call fsync(), the error is allowed to be
>> reported by the next after the next (in other words, (N+2)'th) write() ?
> 
> Yes. The man page is clear on that.

To understand it, I must understand why it is safe to defer error reporting.

> 
>>>> . At least I think that
>>>>
>>>>         spin_lock_irq(&desc->iuspin);
>>>>         we = desc->werr;
>>>>         desc->werr = 0;
>>>>         spin_unlock_irq(&desc->iuspin);
>>>>         if (we < 0)
>>>>                 return usb_translate_errors(we);
>>>>
>>>> in wdm_write() should be moved to after !test_bit(WDM_IN_USE, &desc->flags).
>>>
>>> Why?
>>
>> Otherwise, we can't make sure (N+1)'th write() will report error from N'th write().
> 
> We should move the test for reporting errors later, so that it is sure
> to be carried out? I am afraid I cannot follow that logic.
> 
>> Since I don't know the characteristics of data passed via wdm_write() (I guess that
>> the data is some stateful controlling commands rather than meaningless byte stream),
>> I guess that (N+1)'th wdm_write() attempt should be made only after confirming that
>> N'th wdm_write() attempt received wdm_callback() response. To preserve state / data
>> used by N'th wdm_write() attempt, reporting the error from too late write() attempt
>> would be useless.
> 
> We cannot make assumptions on how user space uses the driver. Somebody
> manually connecting and typing in commands letter by letter must also
> work.

I'm not making any assumptions on how user space uses the driver. Up to now,
your explanation makes me think that you are making assumptions on how user space
uses cdc-wdm driver.

I'm querying you about characteristics of data passed to wdm_write().
Without knowing the difference between writing to cdc-wdm driver and normal file on
some filesystem, I can't judge whether it is acceptable to defer reporting errors.
When an error occurred when writing to normal file on some filesystem, the userspace
would simply treat that file as broken (e.g. delete such file). The userspace of this
cdc-wdm driver might want that any error is immediately reported, for I'm thinking that
each data passed to wdm_write() is stateful, for

  /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */
  #define WDM_DEFAULT_BUFSIZE     256

and

        if (count > desc->wMaxCommand)
                count = desc->wMaxCommand;

makes me think that wdm_write() has to be able to handle chunk of data in atomic
manner.

> 
> We can optimize for the common case, but we must operate according to
> the specs.
>>
>>
>>
>>>> In addition, is
>>>>
>>>>         /* using write lock to protect desc->count */
>>>>         mutex_lock(&desc->wlock);
>>>>
>>>> required? Isn't wdm_mutex that is actually protecting desc->count from modification?
>>>> If it is desc->wlock that is actually protecting desc->count, the !desc->count check
>>>> in wdm_release() and the desc->count == 1 check in wdm_open() have to be done with
>>>> desc->wlock held.
>>>
>>> Correct. So should wdm_mutex be dropped earlier?
>>
>> If recover_from_urb_loss() can tolerate stale desc->count value, wdm_mutex already
> 
> It cannot.
> 
>> protects desc->count. I don't know how this module works. I don't know whether
>> wdm_mutex and/or desc->wlock is held when recover_from_urb_loss() is called from
>> wdm_resume(). It seems that desc->wlock is held but wdm_mutex is not held when
>> recover_from_urb_loss() is called from wdm_post_reset().
> 
> Indeed.

I don't like [RFC 8/8]. Please drop [RFC 8/8] because not only it is unrelated to
hang up problem syzbot is reporting but also lock dependency is still unclear.
The /* using write lock to protect desc->count */ comment is hardly helpful
because wdm_disconnect() accesses desc->count with only wdm_mutex held.
Detailed explanation about why releasing wdm_mutex early in wdm_open() is safe
with regards to e.g. wdm_disconnect()/recover_from_urb_loss()/wdm_release() will
be required for [RFC 8/8].

Also, pointless

+               /* in case flush() had timed out */
+               usb_kill_urb(desc->command);

is still in [RFC 2/8].

Please do squash into one patch, and add detailed/polite patch description like
https://lkml.kernel.org/r/1597188375-4787-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp .
Current series is too fragmented to understand, and nobody can review such series.

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Dienstag, den 22.09.2020, 10:56 +0900 schrieb Tetsuo Handa:
> On 2020/09/21 19:52, Oliver Neukum wrote:

> 
> To understand it, I must understand why it is safe to defer error reporting.

It is not. There is nothing to understand here. If user space needs
a guarantee that data has been pushed out without an error, it will
have to call fsync()

Let me explain.
POSIX, as described in the man page, sets some rules about what
a driver must do and must not do. In other areas it makes
recommendations a good driver should follow. One of them is that
we block as little as possible, at the risk of delaying error
reporting. A good driver will report errors as soon as possible
while being compatible with doing IO asynchronously.

> I'm querying you about characteristics of data passed to wdm_write().
> Without knowing the difference between writing to cdc-wdm driver and normal file on
> some filesystem, I can't judge whether it is acceptable to defer reporting errors.

That is simply not a decision you or I make. The man page clearly
says that it is acceptable. If user space does not like that, it must
call fsync() after write().

> When an error occurred when writing to normal file on some filesystem, the userspace
> would simply treat that file as broken (e.g. delete such file). The userspace of this
> cdc-wdm driver might want that any error is immediately reported, for I'm thinking that
> each data passed to wdm_write() is stateful, for
> 
>   /* CDC-WMC r1.1 requires wMaxCommand to be "at least 256 decimal (0x100)" */
>   #define WDM_DEFAULT_BUFSIZE     256
> 
> and
> 
>         if (count > desc->wMaxCommand)
>                 count = desc->wMaxCommand;
> 
> makes me think that wdm_write() has to be able to handle chunk of data in atomic
> manner.

No. the WDM spec says the clear opposite and the man page of write says
so, too.

> I don't like [RFC 8/8]. Please drop [RFC 8/8] because not only it is unrelated to
> hang up problem syzbot is reporting but also lock dependency is still unclear.

Very well. I will make a patch set that clearly fixes bugs and a
secondary one with optimizations.

	Regards
		Oliver

Re: [RFC 0/5] fix races in CDC-WDM

[Tetsuo Handa]

On 2020/09/22 16:33, Oliver Neukum wrote:
> Am Dienstag, den 22.09.2020, 10:56 +0900 schrieb Tetsuo Handa:
>> On 2020/09/21 19:52, Oliver Neukum wrote:
>> To understand it, I must understand why it is safe to defer error reporting.
> 
> It is not. There is nothing to understand here. If user space needs
> a guarantee that data has been pushed out without an error, it will
> have to call fsync()

>> I'm querying you about characteristics of data passed to wdm_write().
>> Without knowing the difference between writing to cdc-wdm driver and normal file on
>> some filesystem, I can't judge whether it is acceptable to defer reporting errors.
> 
> That is simply not a decision you or I make. The man page clearly
> says that it is acceptable. If user space does not like that, it must
> call fsync() after write().

Then, cdc-wdm driver did not implement fsync() was a big problem. Userspace
needs to be careful not to give up upon -EINVAL when running on older kernels
which did not implement wdm_fsync().

The remaining concern would be how to handle unresponding hardware, for blocking
wdm_write()/wdm_read() and wdm_fsync() are using wait_event_interruptible(). If
the caller do not have a mean to send a signal, the caller might hung up forever
when the hardware stopped responding. Please add a comment that userspace needs to
be careful when calling these functions.

Re: [RFC 0/5] fix races in CDC-WDM

[Oliver Neukum]

Am Dienstag, den 22.09.2020, 17:34 +0900 schrieb Tetsuo Handa:
> On 2020/09/22 16:33, Oliver Neukum wrote:
> > Am Dienstag, den 22.09.2020, 10:56 +0900 schrieb Tetsuo Handa:
> > > On 2020/09/21 19:52, Oliver Neukum wrote:
> > > To understand it, I must understand why it is safe to defer error reporting.
> > 
> > It is not. There is nothing to understand here. If user space needs
> > a guarantee that data has been pushed out without an error, it will
> > have to call fsync()
> > > I'm querying you about characteristics of data passed to wdm_write().
> > > Without knowing the difference between writing to cdc-wdm driver and normal file on
> > > some filesystem, I can't judge whether it is acceptable to defer reporting errors.
> > 
> > That is simply not a decision you or I make. The man page clearly
> > says that it is acceptable. If user space does not like that, it must
> > call fsync() after write().
> 
> Then, cdc-wdm driver did not implement fsync() was a big problem. Userspace
> needs to be careful not to give up upon -EINVAL when running on older kernels
> which did not implement wdm_fsync().

Very well. So I'll call the lack of fsync() a bug, which should be
fixed in stable.

> The remaining concern would be how to handle unresponding hardware, for blocking
> wdm_write()/wdm_read() and wdm_fsync() are using wait_event_interruptible(). If
> the caller do not have a mean to send a signal, the caller might hung up forever
> when the hardware stopped responding. Please add a comment that userspace needs to
> be careful when calling these functions.

wdm_flush() has such a comment. Yet no driver can make a guarantee that
a device will make progress in IO. The driver must, however, provide
a means of dealing with such cases. Usually that means handling
signals. That is the normal semantics of a write() syscall.

I believe we are covered on that.

	Regards
		Oliver