lkml.org 
[lkml]   [2023]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2] PCI: Assign PCI domain by ida_alloc()
    From
    Hi Pali,

    On 14/07/2022 19:41, Pali Rohár wrote:
    > Replace assignment of PCI domain from atomic_inc_return() to ida_alloc().
    >
    > Use two IDAs, one for static domain allocations (those which are defined in
    > device tree) and second for dynamic allocations (all other).
    >
    > During removal of root bus / host bridge release also allocated domain id.
    > So released id can be reused again, for example in situation when
    > dynamically loading and unloading native PCI host bridge drivers.
    >
    > This change also allows to mix static device tree assignment and dynamic by
    > kernel as all static allocations are reserved in dynamic pool.
    >
    > Signed-off-by: Pali Rohár <pali@kernel.org>
    > ---
    > Idea of this patch comes from the following discussion:
    > https://lore.kernel.org/linux-pci/20210412123936.25555-1-pali@kernel.org/t/#u
    >
    > Changes in v2:
    > * Fix broken compilation
    > ---
    > drivers/pci/pci.c | 103 +++++++++++++++++++++++++------------------
    > drivers/pci/probe.c | 5 +++
    > drivers/pci/remove.c | 6 +++
    > include/linux/pci.h | 1 +
    > 4 files changed, 72 insertions(+), 43 deletions(-)
    >
    > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
    > index cfaf40a540a8..34fdcee6634a 100644
    > --- a/drivers/pci/pci.c
    > +++ b/drivers/pci/pci.c
    > @@ -6762,60 +6762,70 @@ static void pci_no_domains(void)
    > }
    >
    > #ifdef CONFIG_PCI_DOMAINS_GENERIC
    > -static atomic_t __domain_nr = ATOMIC_INIT(-1);
    > +static DEFINE_IDA(pci_domain_nr_static_ida);
    > +static DEFINE_IDA(pci_domain_nr_dynamic_ida);
    >
    > -static int pci_get_new_domain_nr(void)
    > +static void of_pci_reserve_static_domain_nr(void)
    > {
    > - return atomic_inc_return(&__domain_nr);
    > + struct device_node *np;
    > + int domain_nr;
    > +
    > + for_each_node_by_type(np, "pci") {
    > + domain_nr = of_get_pci_domain_nr(np);
    > + if (domain_nr < 0)
    > + continue;
    > + /*
    > + * Permanently allocate domain_nr in dynamic_ida
    > + * to prevent it from dynamic allocation.
    > + */
    > + ida_alloc_range(&pci_domain_nr_dynamic_ida,
    > + domain_nr, domain_nr, GFP_KERNEL);
    > + }
    > }
    >
    > static int of_pci_bus_find_domain_nr(struct device *parent)
    > {
    > - static int use_dt_domains = -1;
    > - int domain = -1;
    > + static bool static_domains_reserved = false;
    > + int domain_nr;
    >
    > - if (parent)
    > - domain = of_get_pci_domain_nr(parent->of_node);
    > + /* On the first call scan device tree for static allocations. */
    > + if (!static_domains_reserved) {
    > + of_pci_reserve_static_domain_nr();
    > + static_domains_reserved = true;
    > + }
    > +
    > + if (parent) {
    > + /*
    > + * If domain is in DT then allocate it in static IDA.
    > + * This prevent duplicate static allocations in case
    > + * of errors in DT.
    > + */
    > + domain_nr = of_get_pci_domain_nr(parent->of_node);
    > + if (domain_nr >= 0)
    > + return ida_alloc_range(&pci_domain_nr_static_ida,
    > + domain_nr, domain_nr,
    > + GFP_KERNEL);
    > + }
    >
    > /*
    > - * Check DT domain and use_dt_domains values.
    > - *
    > - * If DT domain property is valid (domain >= 0) and
    > - * use_dt_domains != 0, the DT assignment is valid since this means
    > - * we have not previously allocated a domain number by using
    > - * pci_get_new_domain_nr(); we should also update use_dt_domains to
    > - * 1, to indicate that we have just assigned a domain number from
    > - * DT.
    > - *
    > - * If DT domain property value is not valid (ie domain < 0), and we
    > - * have not previously assigned a domain number from DT
    > - * (use_dt_domains != 1) we should assign a domain number by
    > - * using the:
    > - *
    > - * pci_get_new_domain_nr()
    > - *
    > - * API and update the use_dt_domains value to keep track of method we
    > - * are using to assign domain numbers (use_dt_domains = 0).
    > - *
    > - * All other combinations imply we have a platform that is trying
    > - * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
    > - * which is a recipe for domain mishandling and it is prevented by
    > - * invalidating the domain value (domain = -1) and printing a
    > - * corresponding error.
    > + * If domain was not specified in DT then choose free id from dynamic
    > + * allocations. All domain numbers from DT are permanently in dynamic
    > + * allocations to prevent assigning them to other DT nodes without
    > + * static domain.
    > */
    > - if (domain >= 0 && use_dt_domains) {
    > - use_dt_domains = 1;
    > - } else if (domain < 0 && use_dt_domains != 1) {
    > - use_dt_domains = 0;
    > - domain = pci_get_new_domain_nr();
    > - } else {
    > - if (parent)
    > - pr_err("Node %pOF has ", parent->of_node);
    > - pr_err("Inconsistent \"linux,pci-domain\" property in DT\n");
    > - domain = -1;
    > - }
    > + return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);
    > +}
    >
    > - return domain;
    > +static void of_pci_bus_release_domain_nr(struct pci_bus *bus, struct device *parent)
    > +{
    > + if (bus->domain_nr < 0)
    > + return;
    > +
    > + /* Release domain from ida in which was it allocated. */
    > + if (of_get_pci_domain_nr(parent->of_node) == bus->domain_nr)
    > + ida_free(&pci_domain_nr_static_ida, bus->domain_nr);
    > + else
    > + ida_free(&pci_domain_nr_dynamic_ida, bus->domain_nr);
    > }
    >
    > int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
    > @@ -6823,6 +6833,13 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
    > return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
    > acpi_pci_bus_find_domain_nr(bus);
    > }
    > +
    > +void pci_bus_release_domain_nr(struct pci_bus *bus, struct device *parent)
    > +{
    > + if (!acpi_disabled)
    > + return;
    > + of_pci_bus_release_domain_nr(bus, parent);
    > +}
    > #endif
    >
    > /**
    > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
    > index 17a969942d37..12092d238403 100644
    > --- a/drivers/pci/probe.c
    > +++ b/drivers/pci/probe.c
    > @@ -906,6 +906,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
    > bus->domain_nr = pci_bus_find_domain_nr(bus, parent);
    > else
    > bus->domain_nr = bridge->domain_nr;
    > + if (bus->domain_nr < 0)
    > + goto free;
    > #endif
    >
    > b = pci_find_bus(pci_domain_nr(bus), bridge->busnr);
    > @@ -1030,6 +1032,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
    > device_del(&bridge->dev);
    >
    > free:
    > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
    > + pci_bus_release_domain_nr(bus, parent);
    > +#endif
    > kfree(bus);
    > return err;
    > }
    > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
    > index 4c54c75050dc..0145aef1b930 100644
    > --- a/drivers/pci/remove.c
    > +++ b/drivers/pci/remove.c
    > @@ -160,6 +160,12 @@ void pci_remove_root_bus(struct pci_bus *bus)
    > pci_remove_bus(bus);
    > host_bridge->bus = NULL;
    >
    > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
    > + /* Release domain_nr if it was dynamically allocated */
    > + if (host_bridge->domain_nr == PCI_DOMAIN_NR_NOT_SET)
    > + pci_bus_release_domain_nr(bus, host_bridge->dev.parent);
    > +#endif
    > +
    > /* remove the host bridge */
    > device_del(&host_bridge->dev);
    > }


    After this change was made we are seeing the following bug
    report on a Tegra234 Jetson Orin board ...

    [ 17.172346] tegra194-pcie 141a0000.pcie: host bridge /pcie@141a0000 ranges:
    [ 17.172470] tegra194-pcie 141a0000.pcie: MEM 0x2800000000..0x2b27ffffff -> 0x2800000000
    [ 17.172519] tegra194-pcie 141a0000.pcie: MEM 0x2b28000000..0x2b2fffffff -> 0x0040000000
    [ 17.172548] tegra194-pcie 141a0000.pcie: IO 0x003a100000..0x003a1fffff -> 0x003a100000
    [ 17.173449] tegra194-pcie 141a0000.pcie: iATU: unroll T, 8 ob, 2 ib, align 64K, limit 32G
    [ 18.279048] tegra194-pcie 141a0000.pcie: Phy link never came up
    [ 19.279285] tegra194-pcie 141a0000.pcie: Phy link never came up
    [ 19.279599] tegra194-pcie 141a0000.pcie: PCI host bridge to bus 0005:00
    [ 19.279613] pci_bus 0005:00: root bus resource [bus 00-ff]
    [ 19.279622] pci_bus 0005:00: root bus resource [mem 0x2800000000-0x2b27ffffff pref]
    [ 19.279631] pci_bus 0005:00: root bus resource [mem 0x2b28000000-0x2b2fffffff] (bus address [0x40000000-0x47ffffff])
    [ 19.279639] pci_bus 0005:00: root bus resource [io 0x200000-0x2fffff] (bus address [0x3a100000-0x3a1fffff])
    [ 19.279687] pci 0005:00:00.0: [10de:229a] type 01 class 0x060400
    [ 19.279886] pci 0005:00:00.0: PME# supported from D0 D3hot
    [ 19.283256] pci 0005:00:00.0: PCI bridge to [bus 01-ff]
    [ 19.283590] pcieport 0005:00:00.0: Adding to iommu group 26
    [ 19.283991] pcieport 0005:00:00.0: PME: Signaling with IRQ 174
    [ 19.284429] pcieport 0005:00:00.0: AER: enabled with IRQ 174
    [ 19.285003] pci_bus 0005:01: busn_res: [bus 01-ff] is released
    [ 19.285591] pci 0005:00:00.0: Removing from iommu group 26
    [ 19.285751] pci_bus 0005:00: busn_res: [bus 00-ff] is released
    [ 19.285870] ==================================================================
    [ 19.293351] BUG: KFENCE: use-after-free read in pci_bus_release_domain_nr+0x10/0x70

    [ 19.302817] Use-after-free read at 0x000000007f3b80eb (in kfence-#115):
    [ 19.309677] pci_bus_release_domain_nr+0x10/0x70
    [ 19.309691] dw_pcie_host_deinit+0x28/0x78
    [ 19.309702] tegra_pcie_deinit_controller+0x1c/0x38 [pcie_tegra194]
    [ 19.309734] tegra_pcie_dw_probe+0x648/0xb28 [pcie_tegra194]
    [ 19.309752] platform_probe+0x90/0xd8
    [ 19.309764] really_probe+0xb8/0x298
    [ 19.309777] __driver_probe_device+0x78/0xd8
    [ 19.309788] driver_probe_device+0x38/0x120
    [ 19.309799] __device_attach_driver+0x94/0xe0
    [ 19.309812] bus_for_each_drv+0x70/0xc8
    [ 19.309822] __device_attach+0xfc/0x188
    [ 19.309833] device_initial_probe+0x10/0x18
    [ 19.309844] bus_probe_device+0x94/0xa0
    [ 19.309854] deferred_probe_work_func+0x80/0xb8
    [ 19.309864] process_one_work+0x1e0/0x348
    [ 19.309882] worker_thread+0x48/0x410
    [ 19.309891] kthread+0xf4/0x110
    [ 19.309904] ret_from_fork+0x10/0x20

    [ 19.311457] kfence-#115: 0x00000000063a155a-0x00000000ba698da8, size=1072, cache=kmalloc-2k

    [ 19.311469] allocated by task 96 on cpu 10 at 19.279323s:
    [ 19.311562] __kmem_cache_alloc_node+0x260/0x278
    [ 19.311571] kmalloc_trace+0x24/0x30
    [ 19.311580] pci_alloc_bus+0x24/0xa0
    [ 19.311590] pci_register_host_bridge+0x48/0x4b8
    [ 19.311601] pci_scan_root_bus_bridge+0xc0/0xe8
    [ 19.311613] pci_host_probe+0x18/0xc0
    [ 19.311623] dw_pcie_host_init+0x2c0/0x568
    [ 19.311630] tegra_pcie_dw_probe+0x610/0xb28 [pcie_tegra194]
    [ 19.311647] platform_probe+0x90/0xd8
    [ 19.311653] really_probe+0xb8/0x298
    [ 19.311663] __driver_probe_device+0x78/0xd8
    [ 19.311672] driver_probe_device+0x38/0x120
    [ 19.311682] __device_attach_driver+0x94/0xe0
    [ 19.311694] bus_for_each_drv+0x70/0xc8
    [ 19.311702] __device_attach+0xfc/0x188
    [ 19.311713] device_initial_probe+0x10/0x18
    [ 19.311724] bus_probe_device+0x94/0xa0
    [ 19.311733] deferred_probe_work_func+0x80/0xb8
    [ 19.311743] process_one_work+0x1e0/0x348
    [ 19.311753] worker_thread+0x48/0x410
    [ 19.311763] kthread+0xf4/0x110
    [ 19.311771] ret_from_fork+0x10/0x20

    [ 19.311782] freed by task 96 on cpu 10 at 19.285833s:
    [ 19.311799] release_pcibus_dev+0x30/0x40
    [ 19.311808] device_release+0x30/0x90
    [ 19.311814] kobject_put+0xa8/0x120
    [ 19.311832] device_unregister+0x20/0x30
    [ 19.311839] pci_remove_bus+0x78/0x88
    [ 19.311850] pci_remove_root_bus+0x5c/0x98
    [ 19.311860] dw_pcie_host_deinit+0x28/0x78
    [ 19.311866] tegra_pcie_deinit_controller+0x1c/0x38 [pcie_tegra194]
    [ 19.311883] tegra_pcie_dw_probe+0x648/0xb28 [pcie_tegra194]
    [ 19.311900] platform_probe+0x90/0xd8
    [ 19.311906] really_probe+0xb8/0x298
    [ 19.311916] __driver_probe_device+0x78/0xd8
    [ 19.311926] driver_probe_device+0x38/0x120
    [ 19.311936] __device_attach_driver+0x94/0xe0
    [ 19.311947] bus_for_each_drv+0x70/0xc8
    [ 19.311956] __device_attach+0xfc/0x188
    [ 19.311966] device_initial_probe+0x10/0x18
    [ 19.311976] bus_probe_device+0x94/0xa0
    [ 19.311985] deferred_probe_work_func+0x80/0xb8
    [ 19.311995] process_one_work+0x1e0/0x348
    [ 19.312005] worker_thread+0x48/0x410
    [ 19.312014] kthread+0xf4/0x110
    [ 19.312022] ret_from_fork+0x10/0x20

    [ 19.313579] CPU: 10 PID: 96 Comm: kworker/u24:2 Not tainted 6.2.0 #4
    [ 19.320171] Hardware name: /, BIOS 1.0-d7fb19b 08/10/2022
    [ 19.325852] Workqueue: events_unbound deferred_probe_work_func
    [ 19.331919] ==================================================================

    After reverting this change I no longer see this issue.
    Let me know if you have any thoughts.

    Thanks
    Jon

    --
    nvpublic

    \
     
     \ /
      Last update: 2023-03-27 01:09    [W:4.465 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site