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

[Oliver Neukum <oneukum@suse.com>]

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