From: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
To: linuxppc-dev@lists.ozlabs.org
Subject: [PATCH v2 0/2] powerpc: fix oops in pcibios_release_device() after pcibios_free_controller()
Date: Tue, 9 Aug 2016 21:44:02 -0300 [thread overview]
Message-ID: <1470789844-16401-1-git-send-email-mauricfo@linux.vnet.ibm.com> (raw)
This patchset addresses the problem/suggestion discussed previously [1],
by adding a kref reference counting to the PHB (struct pci_controller):
> It's possible to hit an oops/crash if pcibios_release_device() accesses the
> phb struct and it had been freed earlier -- by pcibios_free_controller() --
> as the memory it pointed to can be reused.
> If after reuse 'phb->controller_ops.release_device' is non-NULL it will be
> called, but it points to an invalid location (that function pointer is not
> set anywhere in the code, so if it's non-NULL, that's not correct), and so
> it hits an oops and the system crashes.
> That problem can happen with the pSeries platform's DLPAR remove operation
> if references to devices are held until after the pcibios_free_controller()
> function runs, and then released - exercising pcibios_release_device() path.
More problem details/call trace are described in the original submission [1].
With the patch applied (tested on 4.8-rc1), the test-case demonstrates that
the PHB is only released/freed after the last reference to the PCI device(s)
is dropped:
Enable debugging messages:
# echo 'file pci-common.c +pf; file pci-hotplug.c +pf' > /sys/kernel/debug/dynamic_debug/control
# echo 8 > /proc/sys/kernel/printk
Hold references to both PCI devices in the PHB:
# 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=$!
Perform DLPAR remove of the PHB:
# drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r
Validating PHB DLPAR capability...yes.
[ 888.776964] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01
[ 888.776983] pci_hp_remove_devices: Removing 0021:01:00.0...
...
[ 893.696431] pci_hp_remove_devices: Removing 0021:01:00.1...
...
[ 908.352717] pci_bus 0021:01: busn_res: [bus 01-ff] is released
[ 908.352744] pcibios_remove_bus: PCI 0021:01, pci_bus c0000001e7d59400, phb c0000001e7d57400
[ 908.352753] controller_put: PCI domain 33, phb c0000001e7d57400
[ 908.352811] pcibios_free_controller: PCI domain 33, phb c0000001e7d57400, phb->is_dynamic 1
[ 908.352820] controller_put: PCI domain 33, phb c0000001e7d57400
[ 908.352832] rpadlpar_io: slot PHB 33 removed
Notice the PHB was not freed yet (controller_free() was not called)
Drop the last references to the PCI devices:
# kill -9 $pid1
[ 991.221998] pcibios_release_device: PCI 0021:01:00.0, pci_dev c0000001ee0b7000, phb c0000001e7d57400
[ 991.222005] controller_put: PCI domain 33, phb c0000001e7d57400
# kill -9 $pid2
[ 996.076293] pcibios_release_device: PCI 0021:01:00.1, pci_dev c0000001ee0b3800, phb c0000001e7d57400
[ 996.076299] controller_put: PCI domain 33, phb c0000001e7d57400
[ 996.076303] controller_free: PCI domain: 33, phb c0000001e7d57400, phb->is_dynamic 1
Notice that only now the PHB was freed.
Note: this patchset currently covers references from struct pci_dev/pci_bus,
which _is_ enough to resolve this particular problem; it does not yet cover
references from struct pci_dn/eeh_pe/eeh_dev (but since those are unchanged
by/unrelated to this patchset, they remain working in the very same manner).
I have gone to great lengths in time studying the relevant code for EEH in
order to implement those too, but am not yet sure of all the details (e.g.,
lifetime of eeh_dev, removal of pci_dn, etc) that need to be considered to
kfree() them - will likely ask Gavin & maintainers for RFC after some time.
Links:
[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-July/145264.html
Changelog:
v2: change approach to use krefs (suggestion by benh & mpe).
Mauricio Faria de Oliveira (2):
powerpc: add refcount to struct pci_controller
powerpc: update pci_controller.refcount for PCI devices and buses
arch/powerpc/include/asm/pci-bridge.h | 15 ++++++++
arch/powerpc/kernel/pci-common.c | 72 +++++++++++++++++++++++++++++++++--
arch/powerpc/kernel/pci-hotplug.c | 29 ++++++++++++++
arch/powerpc/kernel/pci_of_scan.c | 1 +
4 files changed, 114 insertions(+), 3 deletions(-)
--
1.8.3.1
next reply other threads:[~2016-08-10 0:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-10 0:44 Mauricio Faria de Oliveira [this message]
2016-08-10 0:44 ` [PATCH v2 1/2] powerpc: add refcount to struct pci_controller Mauricio Faria de Oliveira
2016-08-10 1:45 ` Andrew Donnellan
2016-08-10 12:03 ` Mauricio Faria de Oliveira
2016-08-10 13:53 ` Mauricio Faria de Oliveira
2016-08-10 21:46 ` Mauricio Faria de Oliveira
2016-08-10 0:44 ` [PATCH v2 2/2] powerpc: update pci_controller.refcount for PCI devices and buses Mauricio Faria de Oliveira
2016-08-10 3:35 ` Andrew Donnellan
2016-08-10 12:30 ` Mauricio Faria de Oliveira
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1470789844-16401-1-git-send-email-mauricfo@linux.vnet.ibm.com \
--to=mauricfo@linux.vnet.ibm.com \
--cc=linuxppc-dev@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).