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

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

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?