| Message ID | 1491627351-1111-4-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Sinan, On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote: > For bridges, have pcie_aspm_init_link_state() allocate a link_state, > regardless of whether it currently has any children. > > pcie_aspm_init_link_state(): Called for bridges (upstream end of > link) after all children have been enumerated. No longer needs to > check aspm_support_enabled or pdev->has_secondary_link or the VIA > quirk: pci_aspm_init() already checked that stuff, so we only need > to check pdev->link_state here. > > Now that we are calling alloc_pcie_link_state() from pci_aspm_init() > , get rid of pci_function_0 function and detect downstream link in > pci_aspm_init_upstream() instead when the function number is 0. > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- > 1 file changed, 36 insertions(+), 36 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index a80d64b..e33f84b 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > } > } > > -/* > - * The L1 PM substate capability is only implemented in function 0 in a > - * multi function device. > - */ > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) > -{ > - struct pci_dev *child; > - > - list_for_each_entry(child, &linkbus->devices, bus_list) > - if (PCI_FUNC(child->devfn) == 0) > - return child; > - return NULL; > -} > - > /* Calculate L1.2 PM substate timing parameters */ > static void aspm_calc_l1ss_info(struct pcie_link_state *link, > struct aspm_register_info *upreg, > @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > INIT_LIST_HEAD(&link->children); > INIT_LIST_HEAD(&link->link); > link->pdev = pdev; > - link->downstream = pci_function_0(pdev->subordinate); > > /* > * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe > @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > static int pci_aspm_init_downstream(struct pci_dev *pdev) > { > + struct pcie_link_state *link; > + struct pci_dev *parent; > + > + /* > + * The L1 PM substate capability is only implemented in function 0 in a > + * multi function device. > + */ > + if (PCI_FUNC(pdev->devfn) != 0) > + return -EINVAL; > + > + parent = pdev->bus->self; > + if (!parent) > + return -EINVAL; > + > + link = parent->link_state; > + link->downstream = pdev; > return 0; > } > > static int pci_aspm_init_upstream(struct pci_dev *pdev) > { > + struct pcie_link_state *link; > + > + link = alloc_pcie_link_state(pdev); > + if (!link) > + return -ENOMEM; This allocates the link_state when we enumerate a Downstream Port instead of when we enumerate a child device. We want the link_state lifetime to match that of the Downstream Port, so this seems good. But we shouldn't at the same time change the link_state deallocation so it happens when the Downstream Port is removed, not when the child device is removed? > return 0; > } > > @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev) > */ > int pci_aspm_init(struct pci_dev *pdev) > { > + if (!aspm_support_enabled) > + return 0; > + > + if (!pci_is_pcie(pdev)) > + return -EINVAL; > + > + /* VIA has a strange chipset, root port is under a bridge */ > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && > + pdev->bus->self) > + return 0; > + > if (!pdev->has_secondary_link) > return pci_aspm_init_downstream(pdev); > > @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > struct pcie_link_state *link; > int blacklist = !!pcie_aspm_sanity_check(pdev); > > - if (!aspm_support_enabled) > - return; > - > - if (pdev->link_state) > - return; > - > - /* > - * We allocate pcie_link_state for the component on the upstream > - * end of a Link, so there's nothing to do unless this device has a > - * Link on its secondary side. > - */ > - if (!pdev->has_secondary_link) > - return; > - > - /* VIA has a strange chipset, root port is under a bridge */ > - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && > - pdev->bus->self) > + if (!pdev->link_state) > return; > > + link = pdev->link_state; > down_read(&pci_bus_sem); > if (list_empty(&pdev->subordinate->devices)) > goto out; > > mutex_lock(&aspm_lock); > - link = alloc_pcie_link_state(pdev); > - if (!link) > - goto unlock; > + > /* > * Setup initial ASPM state. Note that we need to configure > * upstream links also because capable state of them can be > @@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > pcie_set_clkpm(link, policy_to_clkpm_state(link)); > } > > -unlock: > mutex_unlock(&aspm_lock); > out: > up_read(&pci_bus_sem); > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Apr 13, 2017 at 03:48:00PM -0500, Bjorn Helgaas wrote: > Hi Sinan, > > On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote: > > For bridges, have pcie_aspm_init_link_state() allocate a link_state, > > regardless of whether it currently has any children. > > > > pcie_aspm_init_link_state(): Called for bridges (upstream end of > > link) after all children have been enumerated. No longer needs to > > check aspm_support_enabled or pdev->has_secondary_link or the VIA > > quirk: pci_aspm_init() already checked that stuff, so we only need > > to check pdev->link_state here. > > > > Now that we are calling alloc_pcie_link_state() from pci_aspm_init() > > , get rid of pci_function_0 function and detect downstream link in > > pci_aspm_init_upstream() instead when the function number is 0. > > > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > --- > > drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- > > 1 file changed, 36 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index a80d64b..e33f84b 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) > > } > > } > > > > -/* > > - * The L1 PM substate capability is only implemented in function 0 in a > > - * multi function device. > > - */ > > -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) > > -{ > > - struct pci_dev *child; > > - > > - list_for_each_entry(child, &linkbus->devices, bus_list) > > - if (PCI_FUNC(child->devfn) == 0) > > - return child; > > - return NULL; > > -} > > - > > /* Calculate L1.2 PM substate timing parameters */ > > static void aspm_calc_l1ss_info(struct pcie_link_state *link, > > struct aspm_register_info *upreg, > > @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > INIT_LIST_HEAD(&link->children); > > INIT_LIST_HEAD(&link->link); > > link->pdev = pdev; > > - link->downstream = pci_function_0(pdev->subordinate); > > > > /* > > * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe > > @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > > > > static int pci_aspm_init_downstream(struct pci_dev *pdev) > > { > > + struct pcie_link_state *link; > > + struct pci_dev *parent; > > + > > + /* > > + * The L1 PM substate capability is only implemented in function 0 in a > > + * multi function device. > > + */ > > + if (PCI_FUNC(pdev->devfn) != 0) > > + return -EINVAL; > > + > > + parent = pdev->bus->self; > > + if (!parent) > > + return -EINVAL; > > + > > + link = parent->link_state; > > + link->downstream = pdev; > > return 0; > > } > > > > static int pci_aspm_init_upstream(struct pci_dev *pdev) > > { > > + struct pcie_link_state *link; > > + > > + link = alloc_pcie_link_state(pdev); > > + if (!link) > > + return -ENOMEM; > > This allocates the link_state when we enumerate a Downstream Port > instead of when we enumerate a child device. We want the link_state > lifetime to match that of the Downstream Port, so this seems good. > > But we shouldn't at the same time change the link_state deallocation > so it happens when the Downstream Port is removed, not when the child > device is removed? I do see that you change the deallocation in patch [5/5], but I think the deallocation change should be in the same patch as the allocation change. Otherwise I think we have a use-after-free problem in this sequence: # initial enumeration pci_device_add(downstream_port) pci_aspm_init(downstream_port) alloc_pcie_link_state pci_device_add(endpoint) pci_aspm_init(endpoint) # hot-remove endpoint pci_stop_dev(endpoint) pcie_aspm_exit_link_state(endpoint) link = parent->link_state free_link_state(link) # hot-add endpoint pci_aspm_init(endpoint) link = parent->link_state <--- use after free
On 4/13/2017 5:02 PM, Bjorn Helgaas wrote: > I do see that you change the deallocation in patch [5/5], but I think > the deallocation change should be in the same patch as the allocation > change. Otherwise I think we have a use-after-free problem in this > sequence: Sure, I'll reorder. As you can see here, link will be only removed if root port is being removed. Without this, we'll hit the use after free issue you mentioned. if (pdev->has_secondary_link) { link = pdev->link_state; down_read(&pci_bus_sem); mutex_lock(&aspm_lock); list_del(&link->sibling); list_del(&link->link); /* Clock PM is for endpoint device */ free_link_state(link); mutex_unlock(&aspm_lock); up_read(&pci_bus_sem); return; }
On Thu, Apr 13, 2017 at 8:19 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/13/2017 5:02 PM, Bjorn Helgaas wrote: >> I do see that you change the deallocation in patch [5/5], but I think >> the deallocation change should be in the same patch as the allocation >> change. Otherwise I think we have a use-after-free problem in this >> sequence: > > Sure, I'll reorder. As you can see here, link will be only removed if > root port is being removed. > > Without this, we'll hit the use after free issue you mentioned. > > if (pdev->has_secondary_link) { > link = pdev->link_state; > down_read(&pci_bus_sem); > mutex_lock(&aspm_lock); > > list_del(&link->sibling); > list_del(&link->link); > > /* Clock PM is for endpoint device */ > free_link_state(link); > mutex_unlock(&aspm_lock); > up_read(&pci_bus_sem); > return; > } Right, but this "pdev->has_secondary_link" check is in your new code and doesn't show up until patch [5/5]. As of *this* patch [3/5], the link is removed when the endpoint is removed, so we could hit the use-after-free. Granted, we'll only be susceptible to this while bisecting because normally all the patches will be applied. But I think this patch will make more sense and be easier to review if it makes the link_state lifetime match the Port's lifetime. Bjorn
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index a80d64b..e33f84b 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint) } } -/* - * The L1 PM substate capability is only implemented in function 0 in a - * multi function device. - */ -static struct pci_dev *pci_function_0(struct pci_bus *linkbus) -{ - struct pci_dev *child; - - list_for_each_entry(child, &linkbus->devices, bus_list) - if (PCI_FUNC(child->devfn) == 0) - return child; - return NULL; -} - /* Calculate L1.2 PM substate timing parameters */ static void aspm_calc_l1ss_info(struct pcie_link_state *link, struct aspm_register_info *upreg, @@ -798,7 +784,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) INIT_LIST_HEAD(&link->children); INIT_LIST_HEAD(&link->link); link->pdev = pdev; - link->downstream = pci_function_0(pdev->subordinate); /* * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe @@ -828,11 +813,33 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) static int pci_aspm_init_downstream(struct pci_dev *pdev) { + struct pcie_link_state *link; + struct pci_dev *parent; + + /* + * The L1 PM substate capability is only implemented in function 0 in a + * multi function device. + */ + if (PCI_FUNC(pdev->devfn) != 0) + return -EINVAL; + + parent = pdev->bus->self; + if (!parent) + return -EINVAL; + + link = parent->link_state; + link->downstream = pdev; return 0; } static int pci_aspm_init_upstream(struct pci_dev *pdev) { + struct pcie_link_state *link; + + link = alloc_pcie_link_state(pdev); + if (!link) + return -ENOMEM; + return 0; } @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev) */ int pci_aspm_init(struct pci_dev *pdev) { + if (!aspm_support_enabled) + return 0; + + if (!pci_is_pcie(pdev)) + return -EINVAL; + + /* VIA has a strange chipset, root port is under a bridge */ + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && + pdev->bus->self) + return 0; + if (!pdev->has_secondary_link) return pci_aspm_init_downstream(pdev); @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) struct pcie_link_state *link; int blacklist = !!pcie_aspm_sanity_check(pdev); - if (!aspm_support_enabled) - return; - - if (pdev->link_state) - return; - - /* - * We allocate pcie_link_state for the component on the upstream - * end of a Link, so there's nothing to do unless this device has a - * Link on its secondary side. - */ - if (!pdev->has_secondary_link) - return; - - /* VIA has a strange chipset, root port is under a bridge */ - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT && - pdev->bus->self) + if (!pdev->link_state) return; + link = pdev->link_state; down_read(&pci_bus_sem); if (list_empty(&pdev->subordinate->devices)) goto out; mutex_lock(&aspm_lock); - link = alloc_pcie_link_state(pdev); - if (!link) - goto unlock; + /* * Setup initial ASPM state. Note that we need to configure * upstream links also because capable state of them can be @@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) pcie_set_clkpm(link, policy_to_clkpm_state(link)); } -unlock: mutex_unlock(&aspm_lock); out: up_read(&pci_bus_sem);
For bridges, have pcie_aspm_init_link_state() allocate a link_state, regardless of whether it currently has any children. pcie_aspm_init_link_state(): Called for bridges (upstream end of link) after all children have been enumerated. No longer needs to check aspm_support_enabled or pdev->has_secondary_link or the VIA quirk: pci_aspm_init() already checked that stuff, so we only need to check pdev->link_state here. Now that we are calling alloc_pcie_link_state() from pci_aspm_init() , get rid of pci_function_0 function and detect downstream link in pci_aspm_init_upstream() instead when the function number is 0. Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pcie/aspm.c | 72 ++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 36 deletions(-)