From: Mauricio Faria de Oliveira <hidden> Date: 2016-08-10 21:45:34
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 <redacted>
Signed-off-by: Mauricio Faria de Oliveira <redacted>
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(-)
@@ -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);+voidpcibios_host_bridge_release(structpci_host_bridge*bridge)+{+structpci_controller*phb=(structpci_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);+/**Thefunctionisusedtoreturntheminimalalignment*formemoryorI/OwindowsoftheassociatedP2Pbridge.
@@ -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;
From: Mauricio Faria de Oliveira <hidden> Date: 2016-08-10 22:22:04
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
From: Andrew Donnellan <hidden> Date: 2016-08-11 05:01:55
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?
@@ -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);+voidpcibios_host_bridge_release(structpci_host_bridge*bridge)+{+structpci_controller*phb=(structpci_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
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
On Wed, Aug 10, 2016 at 06:45:22PM -0300, Mauricio Faria de Oliveira wrote:
quoted hunk
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 <redacted>
Signed-off-by: Mauricio Faria de Oliveira <redacted>
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(-)
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.
quoted hunk
/*
* The function is used to return the minimal alignment
* for memory or I/O windows of the associated P2P bridge.
From: Andrew Donnellan <hidden> Date: 2016-08-11 07:06:52
On 11/08/16 16:29, Gavin Shan wrote:
On Thu, Aug 11, 2016 at 03:01:44PM +1000, Andrew Donnellan wrote:
quoted
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.
quoted
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
From: Mauricio Faria de Oliveira <hidden> Date: 2016-08-11 17:35:21
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 :)
quoted
Changelog:
Ok, thanks!
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
From: Mauricio Faria de Oliveira <hidden> Date: 2016-08-11 18:00:38
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