linux-kernel - Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 9 Mar 2021 19:07:37 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        shuah@...nel.org, valentina.manea.m@...il.com,
        gregkh@...uxfoundation.org
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races
 leading to gpf

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