Re: [PATCH] usb: use mutex_lock in iowarrior_read()

[Oliver Neukum <oneukum@suse.com>]

Hi,

On 16.09.24 14:44, Jeongjun Park wrote:
> Oliver Neukum <oneukum@suse.com> wrote:
>>
>>
>>
>> On 16.09.24 06:15, Greg KH wrote:
>>> On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:

>>> Please use the guard() form here, it makes the change much simpler and
>>> easier to review and maintain.
>>
>> That would break the O_NONBLOCK case.
>>
>> Looking at the code it indeed looks like iowarrior_read() can race
>> with itself. Strictly speaking it always could happen if a task used
>> fork() after open(). The driver tries to restrict its usage to one
>> thread, but I doubt that the logic is functional.
>>
>> It seems to me the correct fix is something like this:
> 
> Well, I don't know why it's necessary to modify it like this.
> I think it would be more appropriate to patch it to make it
> more maintainable by using guard() as Greg suggested.

Allow me to explain detail.

guard() internally uses mutex_lock(). That means that

a) it will block
b) having blocked it will sleep in the state TASK_UNINTERRUPTIBLE

The driver itself uses TASK_INTERRUPTIBLE in iowarrior_read(),
when it waits for IO. That is entirely correct, as it waits for
an external device doing an operation that may never occur. You
must use TASK_INTERRUPTIBLE.

Now, if you use mutex_lock() to wait for a task waiting for IO
to occur in the state TASK_INTERRUPTIBLE, you are indirectlywaiting for
an event that you must wait for in TASK_INTERRUPTIBLE in the state
TASK_UNINTERRUPTIBLE.
That is a bug. You have created a task that cannot be killed (uid may not match),
but may have to be killed. Furthermore you block even in case the
device has been opened with O_NONBLOCK, which is a second bug.

These limitations are inherent in guard(). Therefore you cannot use
guard here.

	Regards
		Oliver