| Message ID | 9c41a4372b27420c732ff5599d823e363de00c6d.1657704829.git.geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Fix dropping valid root bus resources with .end = zero | expand |
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 ;-( Looking at the commit message and code I'm left with the same questions as Geert added below the '---' line, so further changes might be needed for the coalescing case. Either way without this patch the above if incorrectly skips the root bus resource for us and I think '!res->end' is just incorrect as a check for invalidity. Geert's proposed change on the other hand looks correct to me though I do think that the better solution would be to drop this if and remove the invalidated resource. The loop even uses '..._for_each_entry_safe' so I would assume removing it should be okay? This does look a bit like the expansion case in acpi_pci_root_validate_resources(). For now feel free to add my: Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> Thanks, Niklas [0]: unreferenced object 0x880e20d0 (size 96): comm "swapper/0", pid 1, jiffies 4294937765 (age 3029.430s) hex dump (first 32 bytes): 00 00 03 7f ff b1 ba e0 00 00 03 7f ff b1 ba e0 ................ 00 00 00 00 88 1a 18 30 00 00 00 00 00 00 00 00 .......0........ backtrace: [<00000003c0151e6c>] __kmem_cache_alloc_node+0x204/0x438 [<00000003c00c0a88>] __kmalloc+0x58/0xf0 [<00000003bfe34a9e>] resource_list_create_entry+0x2e/0x60 [<00000003c075ea8a>] pci_add_resource+0x32/0x98 [<00000003bfe22f14>] zpci_bus_device_register+0x124/0x3c0 [<00000003bfe1baf2>] zpci_create_device+0x142/0x1d0 [<00000003bfe1fe4c>] clp_scan_pci_devices+0xfc/0x150 [<00000003c189a468>] pci_base_init+0x148/0x1b0 [<00000003bfdd4ab0>] do_one_initcall+0x78/0x388 [<00000003c189081c>] do_initcalls+0x12c/0x150 [<00000003c1890b1e>] kernel_init_freeable+0x25e/0x2a0 [<00000003c0bb5136>] kernel_init+0x2e/0x170 [<00000003bfdd7914>] __ret_from_fork+0x3c/0x58 [<00000003c0bc79fa>] ret_from_fork+0xa/0x40
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
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
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);
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(-)