linux-kernel - Re: [PATCH v1 3/4] PCI: Change extend_bridge

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: <20200106202301.GA137556@google.com>
Date:   Mon, 6 Jan 2020 14:23:01 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Logan Gunthorpe <logang@...tatee.com>
Subject: Re: [PATCH v1 3/4] PCI: Change extend_bridge_window() to set
 resource size directly

Thanks a lot for splitting these up.  It makes these dramatically
easier to read.

On Mon, Jan 06, 2020 at 03:47:46PM +0000, Nicholas Johnson wrote:
> Change extend_bridge_window() to set resource size directly instead of
> using additional resource lists.
> 
> Because additional resource lists are optional resources, any algorithm
> that requires guaranteed allocation that uses them cannot be guaranteed
> to work.

There is never a guarantee that PCI resource assignment will work.
It's always possible that we don't have enough resources to allow us
to enable a device.  So I'm not sure what this is telling me, and it
doesn't seem like a justification for setting the resource size
directly here.

Prior to this patch, I think all the assignment and changes to
dev->resource[] were in __assign_resources_sorted().  Maybe it's safe
to do some here and some in __assign_resources_sorted(), but I don't
understand it well enough to be confident.

> Remove the resource from add_list, as a zero-sized additional resource
> is redundant.
> 
> Update comment in pci_bus_distribute_available_resources() to reflect
> the above changes.
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
> ---
>  drivers/pci/setup-bus.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index de43815be..0c51f4937 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1836,7 +1836,7 @@ static void adjust_bridge_window(struct pci_dev *bridge, struct resource *res,
>  				 struct list_head *add_list,
>  				 resource_size_t new_size)
>  {
> -	struct pci_dev_resource *dev_res;
> +	resource_size_t add_size;
>  
>  	if (res->parent)
>  		return;
> @@ -1844,17 +1844,10 @@ static void adjust_bridge_window(struct pci_dev *bridge, struct resource *res,
>  	if (resource_size(res) >= new_size)
>  		return;
>  
> -	dev_res = res_to_dev_res(add_list, res);
> -	if (!dev_res)
> -		return;
> -
> -	/* Is there room to extend the window? */
> -	if (new_size - resource_size(res) <= dev_res->add_size)
> -		return;
> -
> -	dev_res->add_size = new_size - resource_size(res);
> -	pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
> -		&dev_res->add_size);
> +	add_size = new_size - resource_size(res);
> +	pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
> +	res->end = res->start + new_size - 1;

How do we know it's safe to extend this, i.e., how do we know there's
nothing immediately after res?

> +	remove_from_list(add_list, res);
>  }
>  
>  static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> @@ -1889,11 +1882,9 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  		mmio_pref.start = min(ALIGN(mmio_pref.start, align),
>  			mmio_pref.end + 1);
>  
> -	/*
> -	 * Update additional resource list (add_list) to fill all the
> -	 * extra resource space available for this port except the space
> -	 * calculated in __pci_bus_size_bridges() which covers all the
> -	 * devices currently connected to the port and below.
> +        /*
> +	 * Now that we have adjusted for alignment, update the bridge window
> +	 * resources to fill as much remaining resource space as possible.
>  	 */
>  	adjust_bridge_window(bridge, io_res, add_list, resource_size(&io));
>  	adjust_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
> -- 
> 2.24.1
>