linux-kernel - Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2a87101f-6bee-4bd1-816a-1dfbe7b4a578@linuxfoundation.org>
Date: Thu, 17 Jul 2025 13:55:14 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Alan Stern <stern@...land.harvard.edu>,
 Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
Cc: Valentina Manea <valentina.manea.m@...il.com>,
 Shuah Khan <shuah@...nel.org>, Hongren Zheng <i@...ithal.me>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Brian G. Merrell" <bgmerrell@...ell.com>, kernel@...labora.com,
 Greg Kroah-Hartman <gregkh@...e.de>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached
 devices

On 7/17/25 12:26, Alan Stern wrote:
> On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
>> The VHCI platform driver aims to forbid entering system suspend when at
>> least one of the virtual USB ports are bound to an active USB/IP
>> connection.
>>
>> However, in some cases, the detection logic doesn't work reliably, i.e.
>> when all devices attached to the virtual root hub have been already
>> suspended, leading to a broken suspend state, with unrecoverable resume.
>>
>> Ensure the attached devices do not enter suspend by setting the syscore
>> PM flag.
>>
>> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>>   drivers/usb/usbip/vhci_hcd.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   				 ctrlreq->wValue, vdev->rhport);
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   
>>   			spin_lock(&vdev->ud.lock);
>> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   			goto out;
> 
> This looks very strange indeed.
> 
> First, why is vhci_urb_enqueue() the right place to do this?  I should
> think you would want to do this just once per device, at the time it is
> attached.  Not every time a new URB is enqueued.

Correct. This isn't the right place to do this even if we want to go with
the option to prevent suspend. The possible place to do this would be
from rh_port_connect() in which case you will have access to usb_hcd device.

This has to be undone from rh_port_disconnect(). Also how does this impact
the usbip_host - we still need to handle usbip_host suspend.

> 
> Second, how do these devices ever go back to being regular non-syscore
> things?
> 
> Third, if this change isn't merely a temporary placeholder, it certainly
> needs to have a comment in the code to explain what it does and why.
> 
> Fourth, does calling dev_pm_syscore_device() really prevent the device
> from going into suspend?  What about runtime suspend?  And what good
> does it to do prevent the device from being suspended if the entire
> server gets suspended?
> 
> Fifth, the patch description says the purpose is to prevent the server
> from going into system suspend.  How does marking some devices with
> dev_pm_syscore_device() accomplish this?
> 

We have been discussing suspend/resume and reboot behavior in another thread
that proposed converting vhci_hcd to use faux bus.

In addition to what Alan is asking, To handle suspend/resume cleanly, the
following has to happen at a higher level:

- Let the usbip hots host know client is suspending the connection.
   The physical device isn't suspended on the host.
- suspend the virtual devices and vhci_hcd

Do the reverse to resume.

I would say:

- We don't want vhci_hcd and usbip_host preventing suspend
- It might be cleaner and safer to detach the devices during
   suspend on both ends. This is similar to what happens now when
   usbip host and vhci_hcd are removed.
- Note that usbip_host and vhci_hcd don't fully support suspend and
   resume at the moment.

thanks,
-- Shuah