Re: [PATCH] PCI: Fix dropping valid root bus resources with .end = zero

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

On Tue, Nov 1, 2022 at 4:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> 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>

Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

Yes I think maybe we should also free it, but I am not entirely sure
it's safe to be freed in this context.
Kai-Heng

>
> 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