linux-kernel - Re: [patch] pci: revert "PCI: remove transparent bridge sizing"

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:	Wed, 26 Mar 2008 14:41:44 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Ivan Kokshaysky <ink@...assic.park.msu.ru>
cc:	Gary Hade <garyhade@...ibm.com>, Ingo Molnar <mingo@...e.hu>,
	Thomas Meyer <thomas@...3r.de>,
	Stefan Richter <stefanr@...6.in-berlin.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	LKML <linux-kernel@...r.kernel.org>,
	Adrian Bunk <bunk@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Natalie Protasevich <protasnb@...il.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	pm@...ian.org
Subject: Re: [patch] pci: revert "PCI: remove transparent bridge sizing"



On Wed, 26 Mar 2008, Ivan Kokshaysky wrote:
>
> PCI: improved sanity check for pdev_sort_resources()
...
> -		if (!r_align) {
> +		if (r_align <= 1) {

Hmm. This makes the code look totally nonsensical.

It seems to come from the fact that we had a very odd way to check bogus 
resources (namely "start == end"), but your code makes it _really_ hard to 
see what is going on.

In fact, I think the old code was buggy too, because we actually *do* have 
single-byte resources where start == end, as showb by google:

	PCI: Ignore bogus resource 1 [3f6:3f6] of Symphony Labs SL82c105
	PCI: Ignore bogus resource 3 [376:376] of Symphony Labs SL82c105

where that resource actually looks valid, and should have a single byte 
alignment! Admittedly I think it was created with a quirk (can you get 
that kind of resource from actually _probing_ a PCI device?) but I do 
think that a single-byte resource is valid.

So I wonder if we shouldn't just make this a bit more readable and also a 
bit more explicit with something like the appended..

NOTE! This will also consider a bridge resource at 0 to be an invalid 
resource (since now the alignment will be zero), which is a bit odd and 
makes me worry a bit. I wouldn't be surprised if some non-PC architectures 
have PCI bridges at zero. But maybe they should be (or already are?) 
marked IORESOURCE_PCI_FIXED?

Ben - the obvious "odd PCI bus resources" architecture would be POWER. Any 
commentary?

		Linus

---
 drivers/pci/setup-res.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 4be7ccf..048ed77 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -211,6 +211,20 @@ int pci_assign_resource_fixed(struct pci_dev *dev, int resno)
 EXPORT_SYMBOL_GPL(pci_assign_resource_fixed);
 #endif
 
+static inline resource_size_t get_resource_alignment(int resno, struct resource *r)
+{
+	resource_size_t start = r->start, end = r->end;
+	resource_size_t alignment = 0;
+
+	/* End == start == 0 - invalid resource */
+	if (end && start <= end) {
+		alignment = end - start - 1;
+		if (resno >= PCI_BRIDGE_RESOURCES)
+			alignment = start;
+	}
+	return alignment;
+}
+
 /* Sort resources by alignment */
 void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 {
@@ -226,10 +240,10 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		if (r->flags & IORESOURCE_PCI_FIXED)
 			continue;
 
-		r_align = r->end - r->start;
-		
-		if (!(r->flags) || r->parent)
+		if (!r->flags || r->parent)
 			continue;
+
+		r_align = get_resource_alignment(i, r);
 		if (!r_align) {
 			printk(KERN_WARNING "PCI: Ignore bogus resource %d "
 				"[%llx:%llx] of %s\n",
@@ -237,7 +251,6 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 				(unsigned long long)r->end, pci_name(dev));
 			continue;
 		}
-		r_align = (i < PCI_BRIDGE_RESOURCES) ? r_align + 1 : r->start;
 		for (list = head; ; list = list->next) {
 			resource_size_t align = 0;
 			struct resource_list *ln = list->next;
--
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/