Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf

From: Shuah Khan <skhan@linuxfoundation.org>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	shuah@kernel.org, valentina.manea.m@gmail.com,
	gregkh@linuxfoundation.org
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf
Date: Tue, 9 Mar 2021 19:07:37 -0700	[thread overview]
Message-ID: <0e8d26dc-d99c-bcaa-1e95-0b5d0e0e535b@linuxfoundation.org> (raw)
In-Reply-To: <47dbbf10-368c-6e45-5eac-c57b75f7ae9c@i-love.sakura.ne.jp>

On 3/9/21 6:02 PM, Tetsuo Handa wrote:
> On 2021/03/10 9:29, Shuah Khan wrote:
>>> It is not a large grain lock. Since event_handler() is exclusively executed, this lock
>>> does _NOT_ block event_handler() unless attach/detach operations run concurrently.
>>>
>>>>
>>
>> event handler queues the events. It shouldn't be blocked by attach
>> and detach. The events could originate for various reasons during
>> the host and vhci operations. I don't like using this lock for
>> attach and detach.
> 
> How can attach/detach deadlock event_handler()?
> event_handler() calls e.g. vhci_shutdown_connection() via ud->eh_ops.shutdown(ud).
> vhci_shutdown_connection() e.g. waits for termination of tx/rx threads via kthread_stop_put().
> event_handler() is already blocked by detach operation.
> How it can make situation worse to wait for creation of tx/rx threads in attach operation?
> 

event_lock shouldn't be held during event ops. usbip_event_add()
uses it to add events. Protecting shutdown path needs a different
approach.

In any case, do you have comments on this patch which doesn't even
touch vhci driver?

I understand you are identifying additional race condition that
the vhci patches in this series might not fix. That doesn't mean
that these patches aren't valid.

Do you have any comments specific to the patches in this series?

thanks,
-- Shuah