linux-kernel - Re: [PATCH] PCI: Do not touch siblings in pci_assign_unassigned_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: <20140630224709.GA22024@google.com>
Date:	Mon, 30 Jun 2014 16:47:09 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Andreas Noever <andreas.noever@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] PCI: Do not touch siblings in
 pci_assign_unassigned_bridge_resources

On Wed, Jun 18, 2014 at 09:41:02PM -0700, Yinghai Lu wrote:
> On Wed, Jun 18, 2014 at 3:40 PM, Andreas Noever
> <andreas.noever@...il.com> wrote:
> > On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <yinghai@...nel.org> wrote:
> > It seems to fix the testcase (no unwanted resources are released). But
> > why do you reassign bus and not just skip the top level bridge? If one
> > of the allocations below bridge failed then a resource of that device
> > will be in fail_res and bridge->subordinate will get released anyways?
> > Also by not removing fail_res from the list you trigger the code in
> > the next loop for the top level bridge (in particular the res->flags =
> > 0 line looks dangerous).
> 
> Should not be dangerous, just second try.

I still don't understand this.  Why do we set "res->flags = 0"?  That
clears out the resource type.  Where do we figure out the type of "res"
again?

> > Could you explain why this function attempts to assign resources two
> > times? In which scenario will a second attempt be successful?
> 
> For example, at first mmio is assigned (by firmware), but pref mmio fails,
> then before second try,  mmio get cleared, then we could separate
> mmio and mmio pref. So need to try again for pref mmio.
> 
> Also I missed one MEM_64 for hotplug path.
> 
> So we need two patches.
> 
> Thanks
> 
> Yinghai

> Subject: [PATCH] pci: Don't release sibiling bridge resources
> 
> On hotplug case, we should not touch sibling bridges that is out
> side of the slots.
> 
> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -1676,10 +1676,16 @@ again:
>  	 * Try to release leaf bridge's resources that doesn't fit resource of
>  	 * child device under that bridge
>  	 */
> -	list_for_each_entry(fail_res, &fail_head, list)
> -		pci_bus_release_bridge_resources(fail_res->dev->bus,
> +	list_for_each_entry(fail_res, &fail_head, list) {
> +		struct pci_bus *bus = fail_res->dev->bus;
> +
> +		if (fail_res->dev == bridge)
> +			bus = bridge->subordinate;
> +
> +		pci_bus_release_bridge_resources(bus,
>  						 fail_res->flags & type_mask,
>  						 whole_subtree);
> +	}
>  
>  	/* restore size and flags */
>  	list_for_each_entry(fail_res, &fail_head, list) {

> Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path
> 
> We miss that in
> |  commit 5b28541552ef5eeffc41d6936105f38c2508e566
> |    PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
> for pci hotplug path.

This changelog is useless.  I don't have time to spend a few hours
figuring out why we want this change.

> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
> 
> ---
>  drivers/pci/setup-bus.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
>  	struct pci_dev_resource *fail_res;
>  	int retval;
>  	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> -				  IORESOURCE_PREFETCH;
> +				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>  
>  again:
>  	__pci_bus_size_bridges(parent, &add_list);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/