[V8,3/5] PCI/ASPM: add init hook to device_add

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

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