linux-kernel - Re: [PATCH] usb: storage: add shutdown function for usb storage driver

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: <5107f6ca-e972-4af1-a21d-6c95778969f3@rowland.harvard.edu>
Date:   Tue, 24 Oct 2023 11:58:37 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "Li, Meng" <Meng.Li@...driver.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "usb-storage@...ts.one-eyed-alien.net" 
        <usb-storage@...ts.one-eyed-alien.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: storage: add shutdown function for usb storage
 driver

On Tue, Oct 24, 2023 at 05:45:40PM +0200, gregkh@...uxfoundation.org wrote:
> On Tue, Oct 24, 2023 at 11:35:19AM -0400, Alan Stern wrote:
> > Okay, that's a different matter.  In fact, I don't know what is supposed 
> > to happen during a clean reboot.
> 
> Define "clean" :)

In this case, I mean what happens when you give the "reboot" command.

> reboot is a system thing that happens before the reboot syscall happens.
> So which are we talking nabout here?
> 
> > Greg, do you know?  Should we take the time to disconnect all the USB 
> > devices during a system shutdown?
> 
> In the past we have not.  And if we switch to do so, we might get some
> complaints as we would now delaying the shutdown process to be longer
> than before.

Yes, that's what I'm afraid of.

> > What happens with non-USB disk drives?  Or other removable devices?
> 
> It would have to come from "above" in the device tree, so does the PCI
> or platform bus say that they should be shut down and their child
> devices?

Well, the PCI layer invokes the HCD's ->shutdown callback.  But the 
usb-storage driver and usbcore don't know this has happened, so they 
start logging errors because they are suddenly unable to communicate 
with a USB drive.  Meng Li is unhappy about these error messages.

Adding a shutdown callback of sorts to usb-storage allows the driver to 
know that it shouldn't communicate with the drive any more, which 
prevents the error message from appearing.  That's what this patch does.  

But that's all it does.  Basically it creates a layering violation just 
to prevent some error messages from showing up in the system log during 
a shutdown or reboot.  The question is whether we want to do this at 
all, and if we do, shouldn't it be handled at the usbcore level rather 
than just within usb-storage?

Alan Stern