[PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Mauricio Faria de Oliveira]

This patch leverages 'struct pci_host_bridge' from the PCI subsystem
in order to free the pci_controller only after the last reference to
its devices is dropped (avoiding an oops in pcibios_release_device()
if the last reference is dropped after pcibios_free_controller()).

The patch relies on pci_host_bridge.release_fn() (and .release_data),
which is called automatically by the PCI subsystem when the root bus
is released (i.e., the last reference is dropped).  Those fields are
set via pci_set_host_bridge_release() (e.g. in the platform-specific
implementation of pcibios_root_bridge_prepare()).

It introduces the 'pcibios_host_bridge_release()' function to be set
as .release_fn(), which expects .release_data to hold the pointer to
the pci_controller to kfree().

It enables that functionality for pseries (although it isn't platform
-specific, and may be used by cxl). It keeps pcibios_free_controller()
backwards-compatible (i.e., kfree(phb) in it) in case no .release_fn()
is defined for the pci_controller.

Details on not-so-elegant design choices:

 - Added 'pci_controller.bridge' field (pointer to associated 'struct
   pci_host_bridge') so *not* to use 'pci_find_host_bridge(phb->bus)'
   in pcibios_free_controller().

   That's because remove_phb_dynamic() sets 'phb->bus = NULL' before
   pcibios_free_controller().  That seems to be very important, with
   commit title 'powerpc/pci: Fix various pseries PCI hotplug issues'
   (so I'll not remove it just to avoid this null pointer dereference).

 - Used 'pci_host_bridge.release_data' field (pointer to associated
   'struct pci_controller') so *not* to 'pci_bus_to_host(bridge->bus)'
   in pcibios_host_bridge_release().

   That's because pci_remove_root_bus() sets 'host_bridge->bus = NULL'
   (so, if the last reference is released after pci_remove_root_bus()
   runs, which eventually reaches pcibios_host_bridge_release(), that
   would hit a null pointer dereference).

   The cxl/vphb.c code calls pci_remove_root_bus(), and the cxl folks
   are interested in this fix.

Test-case:

  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
  <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>

  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
  <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>

  # cat >/dev/sdaa & pid1=$!
  # cat >/dev/sdab & pid2=$!

  # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
  Validating PHB DLPAR capability...yes.
  [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
  [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
  ...
  [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
  ...
  [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
  [  497.072209] rpadlpar_io: slot PHB 33 removed

  # kill -9 $pid1
  # kill -9 $pid2
  [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1

Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com>
Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>

Changelog:
 - v3: different approach: struct pci_host_bridge.release_fn()
 - v2: different approach: struct pci_controller.refcount
---
 arch/powerpc/include/asm/pci-bridge.h |  2 ++
 arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
 arch/powerpc/platforms/pseries/pci.c  |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index b5e88e4..9b11631 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -54,6 +54,7 @@ struct pci_controller_ops {
  */
 struct pci_controller {
 	struct pci_bus *bus;
+	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
 	char is_dynamic;
 #ifdef CONFIG_PPC64
 	int node;
@@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
 /* Allocate & free a PCI host bridge structure */
 extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
 extern void pcibios_free_controller(struct pci_controller *phb);
+extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
 
 #ifdef CONFIG_PCI
 extern int pcibios_vaddr_is_ioport(void __iomem *address);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index a5c0153..c5b5f60 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
 	list_del(&phb->list_node);
 	spin_unlock(&hose_spinlock);
 
-	if (phb->is_dynamic)
+	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
+	if (!phb->bridge->release_fn && phb->is_dynamic)
 		kfree(phb);
 }
 EXPORT_SYMBOL_GPL(pcibios_free_controller);
 
+void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
+{
+	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
+
+	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
+
+	if (phb->is_dynamic)
+		kfree(phb);
+}
+EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);
+
 /*
  * The function is used to return the minimal alignment
  * for memory or I/O windows of the associated P2P bridge.
@@ -1646,6 +1658,7 @@ void pcibios_scan_phb(struct pci_controller *hose)
 		return;
 	}
 	hose->bus = bus;
+	hose->bridge = pci_find_host_bridge(bus);
 
 	/* Get probe mode and perform scan */
 	mode = PCI_PROBE_NORMAL;
diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index fe16a50..146d5da 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -119,6 +119,9 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
 
 	bus = bridge->bus;
 
+	pci_set_host_bridge_release(bridge, pcibios_host_bridge_release,
+					(void *) pci_bus_to_host(bus));
+
 	dn = pcibios_get_phb_of_node(bus);
 	if (!dn)
 		return 0;
-- 
1.8.3.1

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Mauricio Faria de Oliveira]

On 08/10/2016 06:45 PM, Mauricio Faria de Oliveira wrote:
> Changelog:
>  - v3: different approach: struct pci_host_bridge.release_fn()
>  - v2: different approach: struct pci_controller.refcount

Oops, the v3 submission has no cover letter, so the subject changed
a bit from what was in v2.  This is the old subject & archive link:

[PATCH v2 0/2] powerpc: fix oops in pcibios_release_device() after 
pcibios_free_controller() [1]

https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-August/147351.html

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Andrew Donnellan]

On 11/08/16 07:45, Mauricio Faria de Oliveira wrote:
> This patch leverages 'struct pci_host_bridge' from the PCI subsystem
> in order to free the pci_controller only after the last reference to
> its devices is dropped (avoiding an oops in pcibios_release_device()
> if the last reference is dropped after pcibios_free_controller()).
>
> The patch relies on pci_host_bridge.release_fn() (and .release_data),
> which is called automatically by the PCI subsystem when the root bus
> is released (i.e., the last reference is dropped).  Those fields are
> set via pci_set_host_bridge_release() (e.g. in the platform-specific
> implementation of pcibios_root_bridge_prepare()).
>
> It introduces the 'pcibios_host_bridge_release()' function to be set
> as .release_fn(), which expects .release_data to hold the pointer to
> the pci_controller to kfree().
>
> It enables that functionality for pseries (although it isn't platform
> -specific, and may be used by cxl). It keeps pcibios_free_controller()
> backwards-compatible (i.e., kfree(phb) in it) in case no .release_fn()
> is defined for the pci_controller.
>
> Details on not-so-elegant design choices:
>
>  - Added 'pci_controller.bridge' field (pointer to associated 'struct
>    pci_host_bridge') so *not* to use 'pci_find_host_bridge(phb->bus)'
>    in pcibios_free_controller().
>
>    That's because remove_phb_dynamic() sets 'phb->bus = NULL' before
>    pcibios_free_controller().  That seems to be very important, with
>    commit title 'powerpc/pci: Fix various pseries PCI hotplug issues'
>    (so I'll not remove it just to avoid this null pointer dereference).
>
>  - Used 'pci_host_bridge.release_data' field (pointer to associated
>    'struct pci_controller') so *not* to 'pci_bus_to_host(bridge->bus)'
>    in pcibios_host_bridge_release().
>
>    That's because pci_remove_root_bus() sets 'host_bridge->bus = NULL'
>    (so, if the last reference is released after pci_remove_root_bus()
>    runs, which eventually reaches pcibios_host_bridge_release(), that
>    would hit a null pointer dereference).
>
>    The cxl/vphb.c code calls pci_remove_root_bus(), and the cxl folks
>    are interested in this fix.

In cxl, we currently call:

	pci_remove_root_bus(phb->bus);
	pcibios_free_controller(phb);

which appears to break with this patch after I wire up 
pci_set_host_bridge_release() in cxl, as phb can be freed before we call 
pcibios_free_controller().

It seems slightly wrong for me to write:

	struct pci_bus *bus = phb->bus;
	pcibios_free_controller(phb);
	pci_remove_root_bus(bus);

instead.

Thoughts?

>
> Test-case:
>
>   # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
>   <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>
>
>   # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
>   <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>
>
>   # cat >/dev/sdaa & pid1=$!
>   # cat >/dev/sdab & pid2=$!
>
>   # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
>   Validating PHB DLPAR capability...yes.
>   [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
>   [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
>   ...
>   [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
>   ...
>   [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
>   [  497.072209] rpadlpar_io: slot PHB 33 removed
>
>   # kill -9 $pid1
>   # kill -9 $pid2
>   [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1
>
> Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com>
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>

Missing a '---' here :)

> Changelog:
>  - v3: different approach: struct pci_host_bridge.release_fn()
>  - v2: different approach: struct pci_controller.refcount
> ---
>  arch/powerpc/include/asm/pci-bridge.h |  2 ++
>  arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
>  arch/powerpc/platforms/pseries/pci.c  |  3 +++
>  3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index b5e88e4..9b11631 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -54,6 +54,7 @@ struct pci_controller_ops {
>   */
>  struct pci_controller {
>  	struct pci_bus *bus;
> +	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
>  	char is_dynamic;
>  #ifdef CONFIG_PPC64
>  	int node;
> @@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>  /* Allocate & free a PCI host bridge structure */
>  extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
>  extern void pcibios_free_controller(struct pci_controller *phb);
> +extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
>
>  #ifdef CONFIG_PCI
>  extern int pcibios_vaddr_is_ioport(void __iomem *address);
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index a5c0153..c5b5f60 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
>  	list_del(&phb->list_node);
>  	spin_unlock(&hose_spinlock);
>
> -	if (phb->is_dynamic)
> +	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
> +	if (!phb->bridge->release_fn && phb->is_dynamic)
>  		kfree(phb);
>  }
>  EXPORT_SYMBOL_GPL(pcibios_free_controller);
>
> +void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
> +{
> +	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
> +
> +	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
> +
> +	if (phb->is_dynamic)
> +		kfree(phb);
> +}
> +EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);

I figure this is being exported so we can use it in cxl.

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Gavin Shan]

On Thu, Aug 11, 2016 at 03:01:44PM +1000, Andrew Donnellan wrote:
>On 11/08/16 07:45, Mauricio Faria de Oliveira wrote:
>
>In cxl, we currently call:
>
>	pci_remove_root_bus(phb->bus);
>	pcibios_free_controller(phb);
>
>which appears to break with this patch after I wire up
>pci_set_host_bridge_release() in cxl, as phb can be freed before we call
>pcibios_free_controller().
>

pcibios_free_controller() isn't called when bridge's release_fn() is wired
up by pci_set_host_bridge_release(). So you can keep the code as what
you have and no changes needed.

>It seems slightly wrong for me to write:
>
>	struct pci_bus *bus = phb->bus;
>	pcibios_free_controller(phb);
>	pci_remove_root_bus(bus);
>

Yeah, the sequence needs to be reversed. The released PHB instance could
possibly be accessed inside pci_remove_root_bus(). 

Thanks,
Gavin

>
>>
>>Test-case:
>>
>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
>>  <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>
>>
>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
>>  <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>
>>
>>  # cat >/dev/sdaa & pid1=$!
>>  # cat >/dev/sdab & pid2=$!
>>
>>  # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
>>  Validating PHB DLPAR capability...yes.
>>  [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
>>  [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
>>  ...
>>  [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
>>  ...
>>  [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
>>  [  497.072209] rpadlpar_io: slot PHB 33 removed
>>
>>  # kill -9 $pid1
>>  # kill -9 $pid2
>>  [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1
>>
>>Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>>
>
>Missing a '---' here :)
>
>>Changelog:
>> - v3: different approach: struct pci_host_bridge.release_fn()
>> - v2: different approach: struct pci_controller.refcount
>>---
>> arch/powerpc/include/asm/pci-bridge.h |  2 ++
>> arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
>> arch/powerpc/platforms/pseries/pci.c  |  3 +++
>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>>index b5e88e4..9b11631 100644
>>--- a/arch/powerpc/include/asm/pci-bridge.h
>>+++ b/arch/powerpc/include/asm/pci-bridge.h
>>@@ -54,6 +54,7 @@ struct pci_controller_ops {
>>  */
>> struct pci_controller {
>> 	struct pci_bus *bus;
>>+	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
>> 	char is_dynamic;
>> #ifdef CONFIG_PPC64
>> 	int node;
>>@@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>> /* Allocate & free a PCI host bridge structure */
>> extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
>> extern void pcibios_free_controller(struct pci_controller *phb);
>>+extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
>>
>> #ifdef CONFIG_PCI
>> extern int pcibios_vaddr_is_ioport(void __iomem *address);
>>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>index a5c0153..c5b5f60 100644
>>--- a/arch/powerpc/kernel/pci-common.c
>>+++ b/arch/powerpc/kernel/pci-common.c
>>@@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
>> 	list_del(&phb->list_node);
>> 	spin_unlock(&hose_spinlock);
>>
>>-	if (phb->is_dynamic)
>>+	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
>>+	if (!phb->bridge->release_fn && phb->is_dynamic)
>> 		kfree(phb);
>> }
>> EXPORT_SYMBOL_GPL(pcibios_free_controller);
>>
>>+void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
>>+{
>>+	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
>>+
>>+	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
>>+
>>+	if (phb->is_dynamic)
>>+		kfree(phb);
>>+}
>>+EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);
>
>I figure this is being exported so we can use it in cxl.
>
>-- 
>Andrew Donnellan              OzLabs, ADL Canberra
>andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Gavin Shan]

On Wed, Aug 10, 2016 at 06:45:22PM -0300, Mauricio Faria de Oliveira wrote:
>This patch leverages 'struct pci_host_bridge' from the PCI subsystem
>in order to free the pci_controller only after the last reference to
>its devices is dropped (avoiding an oops in pcibios_release_device()
>if the last reference is dropped after pcibios_free_controller()).
>
>The patch relies on pci_host_bridge.release_fn() (and .release_data),
>which is called automatically by the PCI subsystem when the root bus
>is released (i.e., the last reference is dropped).  Those fields are
>set via pci_set_host_bridge_release() (e.g. in the platform-specific
>implementation of pcibios_root_bridge_prepare()).
>
>It introduces the 'pcibios_host_bridge_release()' function to be set
>as .release_fn(), which expects .release_data to hold the pointer to
>the pci_controller to kfree().
>
>It enables that functionality for pseries (although it isn't platform
>-specific, and may be used by cxl). It keeps pcibios_free_controller()
>backwards-compatible (i.e., kfree(phb) in it) in case no .release_fn()
>is defined for the pci_controller.
>
>Details on not-so-elegant design choices:
>
> - Added 'pci_controller.bridge' field (pointer to associated 'struct
>   pci_host_bridge') so *not* to use 'pci_find_host_bridge(phb->bus)'
>   in pcibios_free_controller().
>
>   That's because remove_phb_dynamic() sets 'phb->bus = NULL' before
>   pcibios_free_controller().  That seems to be very important, with
>   commit title 'powerpc/pci: Fix various pseries PCI hotplug issues'
>   (so I'll not remove it just to avoid this null pointer dereference).
>
> - Used 'pci_host_bridge.release_data' field (pointer to associated
>   'struct pci_controller') so *not* to 'pci_bus_to_host(bridge->bus)'
>   in pcibios_host_bridge_release().
>
>   That's because pci_remove_root_bus() sets 'host_bridge->bus = NULL'
>   (so, if the last reference is released after pci_remove_root_bus()
>   runs, which eventually reaches pcibios_host_bridge_release(), that
>   would hit a null pointer dereference).
>
>   The cxl/vphb.c code calls pci_remove_root_bus(), and the cxl folks
>   are interested in this fix.
>
>Test-case:
>
>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
>  <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>
>
>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
>  <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>
>
>  # cat >/dev/sdaa & pid1=$!
>  # cat >/dev/sdab & pid2=$!
>
>  # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
>  Validating PHB DLPAR capability...yes.
>  [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
>  [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
>  ...
>  [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
>  ...
>  [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
>  [  497.072209] rpadlpar_io: slot PHB 33 removed
>
>  # kill -9 $pid1
>  # kill -9 $pid2
>  [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1
>
>Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>
>Changelog:
> - v3: different approach: struct pci_host_bridge.release_fn()
> - v2: different approach: struct pci_controller.refcount
>---
> arch/powerpc/include/asm/pci-bridge.h |  2 ++
> arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
> arch/powerpc/platforms/pseries/pci.c  |  3 +++
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
>diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>index b5e88e4..9b11631 100644
>--- a/arch/powerpc/include/asm/pci-bridge.h
>+++ b/arch/powerpc/include/asm/pci-bridge.h
>@@ -54,6 +54,7 @@ struct pci_controller_ops {
>  */
> struct pci_controller {
> 	struct pci_bus *bus;
>+	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
> 	char is_dynamic;
> #ifdef CONFIG_PPC64
> 	int node;
>@@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
> /* Allocate & free a PCI host bridge structure */
> extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
> extern void pcibios_free_controller(struct pci_controller *phb);
>+extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
>
> #ifdef CONFIG_PCI
> extern int pcibios_vaddr_is_ioport(void __iomem *address);
>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>index a5c0153..c5b5f60 100644
>--- a/arch/powerpc/kernel/pci-common.c
>+++ b/arch/powerpc/kernel/pci-common.c
>@@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
> 	list_del(&phb->list_node);
> 	spin_unlock(&hose_spinlock);
>
>-	if (phb->is_dynamic)
>+	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
>+	if (!phb->bridge->release_fn && phb->is_dynamic)
> 		kfree(phb);
> }
> EXPORT_SYMBOL_GPL(pcibios_free_controller);
>
>+void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
>+{
>+	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
>+
>+	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
>+
>+	if (phb->is_dynamic)
>+		kfree(phb);
>+}
>+EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);
>+

It seems the user has two options here: (1) Setup bridge's release_fn() and call
pcibios_free_controller() explicitly; (2) Call pcibios_free_controller() without
a valid bridge's release_fn() initialized. I think we can provide better interface
to users: what we do in pcibios_free_controller() and pcibios_host_bridge_release()
should be (almost) same. pcibios_host_bridge_release() can be a wrapper of
pcibios_free_controller(). With this, the users have two options: (1) Rely on bridge's
release_fn() to free the PCI controller; (2) Call pcibios_free_controller() as we're
doing currently. Those two options corresponds to immediately or deferred releasing.

> /*
>  * The function is used to return the minimal alignment
>  * for memory or I/O windows of the associated P2P bridge.
>@@ -1646,6 +1658,7 @@ void pcibios_scan_phb(struct pci_controller *hose)
> 		return;
> 	}
> 	hose->bus = bus;
>+	hose->bridge = pci_find_host_bridge(bus);
>
> 	/* Get probe mode and perform scan */
> 	mode = PCI_PROBE_NORMAL;
>diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
>index fe16a50..146d5da 100644
>--- a/arch/powerpc/platforms/pseries/pci.c
>+++ b/arch/powerpc/platforms/pseries/pci.c
>@@ -119,6 +119,9 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
>
> 	bus = bridge->bus;
>
>+	pci_set_host_bridge_release(bridge, pcibios_host_bridge_release,
>+					(void *) pci_bus_to_host(bus));
>+
> 	dn = pcibios_get_phb_of_node(bus);
> 	if (!dn)
> 		return 0;

Thanks,
Gavin

>-- 
>1.8.3.1
>

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Andrew Donnellan]

On 11/08/16 16:29, Gavin Shan wrote:
> On Thu, Aug 11, 2016 at 03:01:44PM +1000, Andrew Donnellan wrote:
>> On 11/08/16 07:45, Mauricio Faria de Oliveira wrote:
>>
>> In cxl, we currently call:
>>
>> 	pci_remove_root_bus(phb->bus);
>> 	pcibios_free_controller(phb);
>>
>> which appears to break with this patch after I wire up
>> pci_set_host_bridge_release() in cxl, as phb can be freed before we call
>> pcibios_free_controller().
>>
>
> pcibios_free_controller() isn't called when bridge's release_fn() is wired
> up by pci_set_host_bridge_release(). So you can keep the code as what
> you have and no changes needed.

Sorry, I didn't mean that pci_set_host_bridge_release() calls 
pcibios_free_controller() - I meant that I added a call to 
pci_set_host_bridge_release() in the cxl vPHB setup.

The use-after-free happens after I call pci_remove_root_bus() and then 
call pcibios_free_controller() afterwards.

>
>> It seems slightly wrong for me to write:
>>
>> 	struct pci_bus *bus = phb->bus;
>> 	pcibios_free_controller(phb);
>> 	pci_remove_root_bus(bus);
>>
>
> Yeah, the sequence needs to be reversed. The released PHB instance could
> possibly be accessed inside pci_remove_root_bus().
>
> Thanks,
> Gavin
>
>>
>>>
>>> Test-case:
>>>
>>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0
>>>  <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...>
>>>
>>>  # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1
>>>  <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...>
>>>
>>>  # cat >/dev/sdaa & pid1=$!
>>>  # cat >/dev/sdab & pid2=$!
>>>
>>>  # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
>>>  Validating PHB DLPAR capability...yes.
>>>  [  479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
>>>  [  479.547049] pci_hp_remove_devices:    Removing 0021:01:00.0...
>>>  ...
>>>  [  483.536303] pci_hp_remove_devices:    Removing 0021:01:00.1...
>>>  ...
>>>  [  497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released
>>>  [  497.072209] rpadlpar_io: slot PHB 33 removed
>>>
>>>  # kill -9 $pid1
>>>  # kill -9 $pid2
>>>  [  506.604458] pcibios_host_bridge_release: domain 33, dynamic 1
>>>
>>> Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
>>>
>>
>> Missing a '---' here :)
>>
>>> Changelog:
>>> - v3: different approach: struct pci_host_bridge.release_fn()
>>> - v2: different approach: struct pci_controller.refcount
>>> ---
>>> arch/powerpc/include/asm/pci-bridge.h |  2 ++
>>> arch/powerpc/kernel/pci-common.c      | 15 ++++++++++++++-
>>> arch/powerpc/platforms/pseries/pci.c  |  3 +++
>>> 3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>>> index b5e88e4..9b11631 100644
>>> --- a/arch/powerpc/include/asm/pci-bridge.h
>>> +++ b/arch/powerpc/include/asm/pci-bridge.h
>>> @@ -54,6 +54,7 @@ struct pci_controller_ops {
>>>  */
>>> struct pci_controller {
>>> 	struct pci_bus *bus;
>>> +	struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */
>>> 	char is_dynamic;
>>> #ifdef CONFIG_PPC64
>>> 	int node;
>>> @@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>>> /* Allocate & free a PCI host bridge structure */
>>> extern struct pci_controller *pcibios_alloc_controller(struct device_node *dev);
>>> extern void pcibios_free_controller(struct pci_controller *phb);
>>> +extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge);
>>>
>>> #ifdef CONFIG_PCI
>>> extern int pcibios_vaddr_is_ioport(void __iomem *address);
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index a5c0153..c5b5f60 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb)
>>> 	list_del(&phb->list_node);
>>> 	spin_unlock(&hose_spinlock);
>>>
>>> -	if (phb->is_dynamic)
>>> +	/* if the associated pci_host_bridge has a release_fn(), rely on that. */
>>> +	if (!phb->bridge->release_fn && phb->is_dynamic)
>>> 		kfree(phb);
>>> }
>>> EXPORT_SYMBOL_GPL(pcibios_free_controller);
>>>
>>> +void pcibios_host_bridge_release(struct pci_host_bridge *bridge)
>>> +{
>>> +	struct pci_controller *phb = (struct pci_controller *) bridge->release_data;
>>> +
>>> +	pr_debug("domain %d, dynamic %d\n", phb->global_number, phb->is_dynamic);
>>> +
>>> +	if (phb->is_dynamic)
>>> +		kfree(phb);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pcibios_host_bridge_release);
>>
>> I figure this is being exported so we can use it in cxl.
>>
>> --
>> Andrew Donnellan              OzLabs, ADL Canberra
>> andrew.donnellan@au1.ibm.com  IBM Australia Limited

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Mauricio Faria de Oliveira]

On 08/11/2016 02:01 AM, Andrew Donnellan wrote:
> In cxl, we currently call:
>
>     pci_remove_root_bus(phb->bus);
>     pcibios_free_controller(phb);
>
> which appears to break with this patch after I wire up
> pci_set_host_bridge_release() in cxl, as phb can be freed before we call
> pcibios_free_controller().

Ugh; you're right. I believe the user is expected to use either one way
or another, but now I see it's not that intuitive -- a design fault.

I'll address this w/ the other review/suggestion by Gavin; replying it.

> Missing a '---' here :)
>
>> Changelog:

Ok, thanks!


-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)

[Mauricio Faria de Oliveira]

Hi Gavin,

tl;dr: thanks for the comments & suggestions; i'll submit v4.

On 08/11/2016 03:40 AM, Gavin Shan wrote: [added some line breaks]
> It seems the user has two options here:

> (1) Setup bridge's release_fn() and call
> pcibios_free_controller() explicitly;

I think the v3 design was non-intuitive in that point -- it does not
seem right for an user to use both options:

if release_fn() is set and is called before pcibios_free_controller()
(normal case w/ DLPAR/PCI hotplug/cxl, as buses/devices are supposed
to be removed before the controller is released) the latter will use
an invalid 'phb' pointer. (what Andrew reported)

In that scenario, it's not even possible for pcibios_free_controller()
to try to detect if release_fn() was already run or not, as the only
information it has is the 'phb' pointer, which may be invalid.

So, I believe the elegant way out of this is your suggestion to have
"immediate or deferred release" and make the user *choose* either one.

Obviously, let's make this explicit to the user -- w/ rename & comments.

 > (2) Call pcibios_free_controller() without
> a valid bridge's release_fn() initialized.

Ok, that looks legitimate for those using immediate release (default).

i.e., once an user decides to use deferred released, it's understood
that pcibios_free_controller() should not be called.

 > I think we can provide better interface
> to users: what we do in pcibios_free_controller() and pcibios_host_bridge_release()
> should be (almost) same. pcibios_host_bridge_release() can be a wrapper of
> pcibios_free_controller().

Right; I implemented only kfree() in pcibios_host_bridge_release()
because I was focused on when it runs *after* pcibios_free_controller();
but it turns out that if it runs *before*, phb becomes invalid pointer.

So, you're right -- both functions are expected to have the same effect
(slightly different code), that is all of what pcibios_free_controller()
does.  The only difference should be the timing. (good point on wrapper)

 > With this, the users have two options: (1) Rely on bridge's
> release_fn() to free the PCI controller; (2) Call pcibios_free_controller() as we're
> doing currently. Those two options corresponds to immediately or deferred releasing.

Looks very good.  I'll submit a v4 like this:
-rename pcibios_host_bridge_release()/pcibios_free_controller_deferred()
-add comments about using _either_ one or another
-pcibios_free_controller_deferred() calls pcibios_free_controller().

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center