Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <9c41a4372b27420c732ff5599d823e363de00c6d.1657704829.git.geert+renesas@glider.be>
 <ed791d5270ce8684085b47d83332ef0aa172e1d0.camel@linux.ibm.com>
In-Reply-To: <ed791d5270ce8684085b47d83332ef0aa172e1d0.camel@linux.ibm.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Tue, 1 Nov 2022 09:23:39 +0100
Message-ID: <CAMuHMdVHf5ydfJtmzkEF89ey-HUM4+vmECDoia0r4YTKvijTkg@mail.gmail.com>
Subject: Re: [PATCH] PCI: Fix dropping valid root bus resources with .end = zero
To:     Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Kai-Heng Feng <kai.heng.feng@canonical.com>,
        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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Niklas,

On Mon, Oct 31, 2022 at 12:56 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> On Wed, 2022-07-13 at 11:35 +0200, Geert Uytterhoeven wrote:
> > 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);
>
> Hi Geert, Hi Bjorn, Hi Kai-Heng,
>
> I just stumbled over this issue on s390 with the below kmemleak
> splat[0]. On s390 we currently always have a single PCI bus with bus
> number 00 per PCI domain so this is triggered whenever there are PCI
> devices attached to the system.
>
> Applying the patch from this mail makes the splat go away and the
> 'pci_bus 0002:00: root bus resource [bus 00]' message reappear. As this
> mail is from July I guess it got lost and this was never picked up ;-(

Sorry, I still have to go over all patches submitted last summer that
didn't make it...

> For now feel free to add my:
>
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds