linux-kernel - Re: [PATCH v2] PCI: avoid bridge feature re-probing on hotplug

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:   Thu, 20 Dec 2018 13:49:50 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     linux-kernel@...r.kernel.org, xuyandong <xuyandong2@...wei.com>,
        stable@...r.kernel.org, Yinghai Lu <yinghai@...nel.org>,
        Jesse Barnes <jbarnes@...tuousgeek.org>,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH v2] PCI: avoid bridge feature re-probing on hotplug

Hi Michael,

On Mon, Dec 17, 2018 at 07:45:41PM -0500, Michael S. Tsirkin wrote:
> commit 1f82de10d6b1 ("PCI/x86: don't assume prefetchable ranges are 64bit")
> added probing of bridge support for 64 bit memory each time bridge is
> re-enumerated.
> 
> Unfortunately this probing is destructive if any device behind
> the bridge is in use at this time.
> 
> This was observed in the field, see
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01711.html
> and specifically
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02082.html
> 
> There's no real need to re-probe the bridge features as the
> registers in question never change - detect that using
> the memory flag being set (it's always set on the 1st pass since
> all PCI2PCI bridges support memory forwarding) and skip the probing.
> Thus, only the first call will perform the disruptive probing and sets
> the resource flags as required - which we can be reasonably sure happens
> before any devices have been configured.
> Avoiding repeated calls to pci_bridge_check_ranges might be even nicer.
> Unfortunately I couldn't come up with a clean way to do it without a
> major probing code refactoring.

I'm OK with major probe code refactoring as long as it's done
carefully.  Doing a special-case fix like this solves the immediate
problem but adds to the long-term maintenance problem.

As far as I can tell, everything in pci_bridge_check_ranges() should
be done once at enumeration-time, e.g., in the pci_read_bridge_bases()
path, and pci_bridge_check_ranges() itself should be removed.

If that turns out to be impossible for some reason, we need a comment
explaining why.

> Reported-by: xuyandong <xuyandong2@...wei.com>
> Tested-by: xuyandong <xuyandong2@...wei.com>
> Cc: stable@...r.kernel.org
> Cc: Yinghai Lu <yinghai@...nel.org>
> Cc: Jesse Barnes <jbarnes@...tuousgeek.org>
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> ---
> 
> Please review and consider for stable.
> 
> changes from v1:
> 	comment and commit log updates to address comments by Bjorn.
> 
>  drivers/pci/setup-bus.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index ed960436df5e..d5c25d465d97 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -741,6 +741,16 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
>  	struct resource *b_res;
>  
>  	b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
> +
> +	/*
> +	 * Don't re-check after this was called once already:
> +	 * important since bridge might be in use.
> +	 * Note: this is only reliable because as per spec all PCI to PCI
> +	 * bridges support memory unconditionally so IORESOURCE_MEM is set.
> +	 */
> +	if (b_res[1].flags & IORESOURCE_MEM)
> +		return;
> +
>  	b_res[1].flags |= IORESOURCE_MEM;
>  
>  	pci_read_config_word(bridge, PCI_IO_BASE, &io);
> -- 
> MST