Messages in this thread | | | Date | Mon, 20 Mar 2023 20:26:05 +0000 | Subject | Re: [PATCH v2] PCI: Assign PCI domain by ida_alloc() | From | Jon Hunter <> |
| |
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
| |