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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <42bcf1e1-1bb2-4b63-9790-61393f780202@rowland.harvard.edu>
Date: Thu, 17 Jul 2025 14:26:13 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: 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
Subject: Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached
 devices

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.

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?

Alan Stern