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

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

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.