All of lore.kernel.org
 help / color / mirror / Atom feed
From: Geert Uytterhoeven <geert+renesas@glider.be>
To: Bjorn Helgaas <bhelgaas@google.com>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: Johannes Berg <johannes@sipsolutions.net>,
	Rob Herring <robh@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	linux-pci@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Geert Uytterhoeven <geert+renesas@glider.be>
Subject: [PATCH] PCI: Fix dropping valid root bus resources with .end = zero
Date: Wed, 13 Jul 2022 11:35:50 +0200	[thread overview]
Message-ID: <9c41a4372b27420c732ff5599d823e363de00c6d.1657704829.git.geert+renesas@glider.be> (raw)

On r8a7791/koelsch:

    kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
    # cat /sys/kernel/debug/kmemleak
    unreferenced object 0xc3a34e00 (size 64):
      comm "swapper/0", pid 1, jiffies 4294937460 (age 199.080s)
      hex dump (first 32 bytes):
	b4 5d 81 f0 b4 5d 81 f0 c0 b0 a2 c3 00 00 00 00  .]...]..........
	00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
      backtrace:
	[<fe3aa979>] __kmalloc+0xf0/0x140
	[<34bd6bc0>] resource_list_create_entry+0x18/0x38
	[<767046bc>] pci_add_resource_offset+0x20/0x68
	[<b3f3edf2>] devm_of_pci_get_host_bridge_resources.constprop.0+0xb0/0x390

When coalescing two resources for a contiguous aperture, the first
resource is enlarged to cover the full contiguous range, while the
second resource is marked invalid.  This invalidation is done by
clearing the flags, start, and end members.

When adding the initial resources to the bus later, invalid resources
are skipped.  Unfortunately, the check for an invalid resource considers
only the end member, causing false positives.

E.g. on r8a7791/koelsch, root bus resource 0 ("bus 00") is skipped, and
no longer registered with pci_bus_insert_busn_res() (causing the memory
leak), nor printed:

     pci-rcar-gen2 ee090000.pci: host bridge /soc/pci@ee090000 ranges:
     pci-rcar-gen2 ee090000.pci:      MEM 0x00ee080000..0x00ee08ffff -> 0x00ee080000
     pci-rcar-gen2 ee090000.pci: PCI: revision 11
     pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
    -pci_bus 0000:00: root bus resource [bus 00]
     pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff]

Fix this by only skipping resources where all of the flags, start, and
end members are zero.

Fixes: 7c3855c423b17f6c ("PCI: Coalesce host bridge contiguous apertures")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is there any side effect of not registering the root bus resource with
pci_bus_insert_busn_res()?  This is the resource created by
of_pci_parse_bus_range(), and thus affects any DT platforms using
"bus-range = <0 0>".

Perhaps checking for "!res->flags" would be sufficient?

I assume this still causes memory leaks on systems where resources are
coalesced, as the second resource of a contiguous aperture is no longer
referenced? Perhaps instead of clearing the resource, it should be
removed from the list (and freed? is it actually safe to do that?)?

Apparently Johannes had identified the bug before, but didn't realize
the full impact...
https://lore.kernel.org/r/5331e942ff28bb191d62bb403b03ceb7d750856c.camel@sipsolutions.net/
---
 drivers/pci/probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37033a..be628798d279ada0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -994,7 +994,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	resource_list_for_each_entry_safe(window, n, &resources) {
 		offset = window->offset;
 		res = window->res;
-		if (!res->end)
+		if (!res->flags && !res->start && !res->end)
 			continue;
 
 		list_move_tail(&window->node, &bridge->windows);
-- 
2.25.1


             reply	other threads:[~2022-07-13  9:36 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-13  9:35 Geert Uytterhoeven [this message]
2022-10-31 11:56 ` [PATCH] PCI: Fix dropping valid root bus resources with .end = zero Niklas Schnelle
2022-11-01  8:23   ` Geert Uytterhoeven
2022-11-07  3:06     ` Kai-Heng Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9c41a4372b27420c732ff5599d823e363de00c6d.1657704829.git.geert+renesas@glider.be \
    --to=geert+renesas@glider.be \
    --cc=bhelgaas@google.com \
    --cc=johannes@sipsolutions.net \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.