linux-kernel - Re: [RFC PATCH] PCI: Detach driver before procfs & sysfs teardown on device remove

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] [day] [month] [year] [list]

Message-ID: <20171025135116.GM21840@bhelgaas-glaptop.roam.corp.google.com>
Date:   Wed, 25 Oct 2017 08:51:16 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     linux-pci@...r.kernel.org, bhelgaas@...gle.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] PCI: Detach driver before procfs & sysfs teardown on
 device remove

On Wed, Oct 11, 2017 at 03:35:56PM -0600, Alex Williamson wrote:
> When removing a device, for example a VF being removed due to SR-IOV
> teardown, a "soft" hot-unplug via 'echo 1 > remove' in sysfs, or an
> actual hot-unplug, we first remove the procfs and sysfs attributes
> for the device before attempting to release the device from any driver
> bound to it.  Unbinding the driver from the device can take time.  The
> device might need to write out data or it might be actively in use.
> If it's in use by userspace through a vfio driver, the unbind might
> block until the user releases the device.  This leads to a potentially
> non-trivial amount of time where the device exists, but we've torn
> down the interfaces that userspace uses to examine devices, for
> instance lspci might generate this sort of error:
> 
> pcilib: Cannot open /sys/bus/pci/devices/0000:01:0a.3/config
> lspci: Unable to read the standard configuration space header of device 0000:01:0a.3
> 
> We don't seem to have any dependence on this teardown ordering in the
> kernel, so let's unbind the driver first, which is also more symmetric
> with the instantiation of the device.
> 
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
> 
> Am I missing any reason for the existing ordering?  Looking through
> history, it seems that we've simply always had this ordering.  We're
> dealing only with pci related device attributes, so I can't figure
> how the current ordering protects us from any races.  Anyway, I'd
> appreciate comments if there's something obvious I'm missing.  Thanks.

Makes sense to me.  Applied to pci/virtualization for v4.15, thanks!

>  drivers/pci/remove.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 73a03d382590..2fa0dbde36b7 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -19,9 +19,9 @@ static void pci_stop_dev(struct pci_dev *dev)
>  	pci_pme_active(dev, false);
>  
>  	if (dev->is_added) {
> +		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> -		device_release_driver(&dev->dev);
>  		dev->is_added = 0;
>  	}
>  
>