PCI/AER: Use-after-free fix
cover PCI/AER: Use-after-free fix
AER error handling walks the PCI topology below a root port, saving pointers of the pci_dev structs affected by the error along the way.
At the same time, the pcie hotplug driver could be freeing those very same structures, causing the AER driver to reference freed memory.
This series fixes this by synchroniziing the aer driver with the pci hotplug driver during. The final patch is the one that ultimately provides the locking by having AER lock the same pci lock rescan/remove mutex as the pciehp driver. The first three patches are prepping to make it safe for the aer bottom half handler to hold that lock.
Keith Busch (4):
PCI/AER: Remove unused parameters PCI/AER: Replace struct pcie_device with pci_dev PCI/AER: Reference count aer structures PCI/AER: Lock pci topology when scanning errors
drivers/pci/pcie/aer/aerdrv.c | 28 +++++++++++++++++++++------- drivers/pci/pcie/aer/aerdrv.h | 9 +++------ drivers/pci/pcie/aer/aerdrv_core.c | 38 +++++++++++++++++--------------------- 3 files changed, 41 insertions(+), 34 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/4 PCI/AER: Remove unused parameters
--- drivers/pci/pcie/aer/aerdrv_core.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/pci/pcie/aer/aerdrv_core.c +5 -9
@@ -541,15 +541,12 @@ static void do_recovery(struct pci_dev *dev, int severity)/** * handle_error_source - handle logging error into an event log -* @aerdev: pointer to pcie_device data structure of the root port * @dev: pointer to pci_dev data structure of error source device * @info: comprehensive error information * * Invoked when an error being detected by Root Port. */ -static void handle_error_source (struct pcie_device *aerdev, -struct pci_dev *dev, -struct aer_err_info *info) +static void handle_error_source (struct pci_dev *dev,struct aer_err_info *info) {int pos;
@@ -695,8 +692,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)return 1 ; } -static inline void aer_process_err_devices (struct pcie_device *p_device, -struct aer_err_info *e_info) +static inline void aer_process_err_devices (struct aer_err_info *e_info) {int i;
@@ -707,7 +703,7 @@ static inline void aer_process_err_devices(struct pcie_device *p_device, }for (i =0 ; i < e_info->error_dev_num && e_info->dev[i]; i++) {if (get_device_error_info (e_info->dev[i], e_info)) -handle_error_source (p_device, e_info->dev[i], e_info); +handle_error_source (e_info->dev[i], e_info); } }
@@ -738,7 +734,7 @@ static void aer_isr_one_error(struct pcie_device *p_device,aer_print_port_info (p_device->port, e_info);if (find_source_device (p_device->port, e_info)) -aer_process_err_devices (p_device, e_info); +aer_process_err_devices (e_info); }if (e_src->status &PCI_ERR_ROOT_UNCOR_RCV ) {
@@ -757,7 +753,7 @@ static void aer_isr_one_error(struct pcie_device *p_device,aer_print_port_info (p_device->port, e_info);if (find_source_device (p_device->port, e_info)) -aer_process_err_devices (p_device, e_info); +aer_process_err_devices (e_info); } }
Signature · 1 line
2.14.3
PATCH 2/4 PCI/AER: Replace struct pcie_device with pci_dev
In reply to Keith BuschThe AER driver really only needed the pcie_device to get to the port pci_dev, so this patch removes the unnecessary indirection.
--- drivers/pci/pcie/aer/aerdrv.c | 6 +++--- drivers/pci/pcie/aer/aerdrv.h | 2 +- drivers/pci/pcie/aer/aerdrv_core.c | 18 ++++++++---------- 3 files changed, 12 insertions(+), 14 deletions(-)
drivers/pci/pcie/aer/aerdrv.c +3 -3
@@ -94,7 +94,7 @@ static void set_downstream_devices_error_reporting(struct pci_dev *dev,*/ static void aer_enable_rootport (struct aer_rpc *rpc) { -struct pci_dev *pdev = rpc->rpd->port; +struct pci_dev *pdev = rpc->rpd;int aer_pos;u16 reg16;u32 reg32;
@@ -136,7 +136,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)*/ static void aer_disable_rootport (struct aer_rpc *rpc) { -struct pci_dev *pdev = rpc->rpd->port; +struct pci_dev *pdev = rpc->rpd;u32 reg32;int pos;
@@ -232,7 +232,7 @@ static struct aer_rpc *aer_alloc_rpc(struct pcie_device *dev)/* Initialize Root lock access, e_lock, to Root Error Status Reg */ spin_lock_init (&rpc->e_lock); - rpc->rpd = dev; + rpc->rpd = dev->port;INIT_WORK (&rpc->dpc_handler, aer_isr);mutex_init (&rpc->rpc_mutex);
drivers/pci/pcie/aer/aerdrv.h +1 -1
@@ -58,7 +58,7 @@ struct aer_err_source { };struct aer_rpc { -struct pcie_device *rpd;/* Root Port device */ +struct pci_dev *rpd;/* Root Port device */ struct work_struct dpc_handler;struct aer_err_source e_sources[AER_ERROR_SOURCES_MAX ];struct aer_err_info e_info;
drivers/pci/pcie/aer/aerdrv_core.c +8 -10
@@ -709,13 +709,13 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)/** * aer_isr_one_error - consume an error detected by root port -* @p_device: pointer to error root port service device +* @rpc: pointer to the root port which holds an error * @e_src: pointer to an error source */ -static void aer_isr_one_error (struct pcie_device *p_device, +static void aer_isr_one_error (struct aer_rpc *rpc,struct aer_err_source *e_src) { -struct aer_rpc *rpc =get_service_data (p_device); +struct pci_dev *pdev = rpc->rpd;struct aer_err_info *e_info = &rpc->e_info;/*
@@ -730,10 +730,9 @@ static void aer_isr_one_error(struct pcie_device *p_device, e_info->multi_error_valid =1 ; else e_info->multi_error_valid = 0; +aer_print_port_info (pdev, e_info); -aer_print_port_info (p_device->port, e_info); - -if (find_source_device (p_device->port, e_info)) +if (find_source_device (pdev, e_info))aer_process_err_devices (e_info); }
@@ -750,9 +749,9 @@ static void aer_isr_one_error(struct pcie_device *p_device,else e_info->multi_error_valid =0 ; -aer_print_port_info (p_device->port, e_info); +aer_print_port_info (pdev, e_info); -if (find_source_device (p_device->port, e_info)) +if (find_source_device (pdev, e_info))aer_process_err_devices (e_info); } }
@@ -795,11 +794,10 @@ static int get_e_source(struct aer_rpc *rpc, struct aer_err_source *e_src)void aer_isr (struct work_struct *work) { struct aer_rpc *rpc = container_of(work, struct aer_rpc, dpc_handler); -struct pcie_device *p_device = rpc->rpd;struct aer_err_source uninitialized_var (e_src );mutex_lock (&rpc->rpc_mutex);while (get_e_source (rpc, &e_src)) -aer_isr_one_error (p_device, &e_src); +aer_isr_one_error (rpc, &e_src);mutex_unlock (&rpc->rpc_mutex); }
Signature · 1 line
2.14.3
PATCH 3/4 PCI/AER: Reference count aer structures
In reply to Keith BuschThe AER driver's removal was flushing its scheduled work to ensure it was safe to free the aer structure. This patch removes that flushing and prevents use-after-free instead by reference counting the aer root port structure and its pci_dev.
The purpose of this patch is to allow the bottom half worker to take locks that may be held while the aer driver's removal is called.
--- drivers/pci/pcie/aer/aerdrv.c | 23 +++++++++++++++++++---- drivers/pci/pcie/aer/aerdrv.h | 2 ++ drivers/pci/pcie/aer/aerdrv_core.c | 2 ++ 3 files changed, 23 insertions(+), 4 deletions(-)
drivers/pci/pcie/aer/aerdrv.c +19 -4
@@ -209,7 +209,9 @@ irqreturn_t aer_irq(int irq, void *context)spin_unlock_irqrestore (&rpc->e_lock, flags);/* Invoke DPC handler */ -schedule_work (&rpc->dpc_handler); +kref_get (&rpc->ref); +if (!schedule_work (&rpc->dpc_handler)) +aer_release (rpc);return IRQ_HANDLED ; }
@@ -232,7 +234,8 @@ static struct aer_rpc *aer_alloc_rpc(struct pcie_device *dev)/* Initialize Root lock access, e_lock, to Root Error Status Reg */ spin_lock_init (&rpc->e_lock); - rpc->rpd = dev->port; + rpc->rpd =pci_dev_get (dev->port); +kref_init (&rpc->ref);INIT_WORK (&rpc->dpc_handler, aer_isr);mutex_init (&rpc->rpc_mutex);
@@ -242,6 +245,19 @@ static struct aer_rpc *aer_alloc_rpc(struct pcie_device *dev)return rpc; } +static void aer_free (struct kref *ref) +{ + struct aer_rpc *rpc = container_of(ref, struct aer_rpc, ref); + +pci_dev_put (rpc->rpd); +kfree (rpc); +} + +void aer_release (struct aer_rpc *rpc) +{ +kref_put (&rpc->ref, aer_free); +} +/** * aer_remove - clean up resources * @dev: pointer to the pcie_dev data structure
@@ -257,10 +273,9 @@ static void aer_remove(struct pcie_device *dev)if (rpc->isr)free_irq (dev->irq, dev); -flush_work (&rpc->dpc_handler);aer_disable_rootport (rpc); -kfree (rpc);set_service_data (dev,NULL ); +aer_release (rpc); } }
drivers/pci/pcie/aer/aerdrv.h +2 -0
@@ -60,6 +60,7 @@ struct aer_err_source {struct aer_rpc {struct pci_dev *rpd;/* Root Port device */ struct work_struct dpc_handler; +struct kref ref;struct aer_err_source e_sources[AER_ERROR_SOURCES_MAX ];struct aer_err_info e_info;unsigned short prod_idx;/* Error Producer Index */
@@ -110,6 +111,7 @@ extern struct bus_type pcie_port_bus_type;void aer_isr (struct work_struct *work);void aer_print_error (struct pci_dev *dev,struct aer_err_info *info);void aer_print_port_info (struct pci_dev *dev,struct aer_err_info *info); +void aer_release (struct aer_rpc *rpc);irqreturn_t aer_irq (int irq,void *context);#ifdef CONFIG_ACPI_APEI
drivers/pci/pcie/aer/aerdrv_core.c +2 -0
@@ -800,4 +800,6 @@ void aer_isr(struct work_struct *work)while (get_e_source (rpc, &e_src))aer_isr_one_error (rpc, &e_src);mutex_unlock (&rpc->rpc_mutex); + +aer_release (rpc); }
Signature · 1 line
2.14.3
PATCH 4/4 PCI/AER: Lock pci topology when scanning errors
In reply to Keith BuschThe side effects of surprise removal may trigger AER handling. The AER handling walks the pci topology and may access a pci_dev that is being freed by the hotplug handler.
This patch fixes that use-after-free by locking the PCI topology in the AER handler so it isn't racing with the pciehp removal.
Since the AER handler now runs under a global PCI lock, the rpc specific mutex is no longer necessary.
--- drivers/pci/pcie/aer/aerdrv.c | 1 - drivers/pci/pcie/aer/aerdrv.h | 5 ----- drivers/pci/pcie/aer/aerdrv_core.c | 4 ++-- 3 files changed, 2 insertions(+), 8 deletions(-)
drivers/pci/pcie/aer/aerdrv.c +0 -1
@@ -237,7 +237,6 @@ static struct aer_rpc *aer_alloc_rpc(struct pcie_device *dev) rpc->rpd =pci_dev_get (dev->port);kref_init (&rpc->ref);INIT_WORK (&rpc->dpc_handler, aer_isr); -mutex_init (&rpc->rpc_mutex);/* Use PCIe bus function to store rpc into PCIe device */ set_service_data (dev, rpc);
drivers/pci/pcie/aer/aerdrv.h +0 -5
@@ -70,11 +70,6 @@ struct aer_rpc {* Lock access to Error Status/ID Regs * and error producer/consumer index */ -struct mutex rpc_mutex;/* -* only one thread could do -* recovery on the same -* root port hierarchy -*/ };struct aer_broadcast_data {
drivers/pci/pcie/aer/aerdrv_core.c +2 -2
@@ -796,10 +796,10 @@ void aer_isr(struct work_struct *work) struct aer_rpc *rpc = container_of(work, struct aer_rpc, dpc_handler);struct aer_err_source uninitialized_var (e_src ); -mutex_lock (&rpc->rpc_mutex); +pci_lock_rescan_remove ();while (get_e_source (rpc, &e_src))aer_isr_one_error (rpc, &e_src); -mutex_unlock (&rpc->rpc_mutex); +pci_unlock_rescan_remove ();aer_release (rpc); }
Signature · 1 line
2.14.3
Re: [PATCH 4/4] PCI/AER: Lock pci topology when scanning errors
Quoted 51 lines · Keith Busch
The side effects of surprise removal may trigger AER handling. The AER handling walks the pci topology and may access a
pci_devthat is being freed by the hotplug handler.This patch fixes that use-after-free by locking the PCI topology in the AER handler so it isn't racing with the pciehp removal.
Since the AER handler now runs under a global PCI lock, the rpc specific mutex is no longer necessary.
Reported-by Alex Gagniuc <[email protected]>Signed-off-by Keith Busch <[email protected]>--- drivers/pci/pcie/aer/aerdrv.c | 1 - drivers/pci/pcie/aer/aerdrv.h | 5 ----- drivers/pci/pcie/aer/aerdrv_core.c | 4 ++-- 3 files changed, 2 insertions(+), 8 deletions(-)drivers/pci/pcie/aer/aerdrv.c +0
-1@@ -237,7 +237,6 @@ static struct aer_rpc *aer_alloc_rpc(struct pcie_device *dev) rpc->rpd =pci_dev_get (dev->port);kref_init (&rpc->ref);INIT_WORK (&rpc->dpc_handler, aer_isr); -mutex_init (&rpc->rpc_mutex);/* Use PCIe bus function to store rpc into PCIe device */ set_service_data (dev, rpc);drivers/pci/pcie/aer/aerdrv.h +0
-5@@ -70,11 +70,6 @@ struct aer_rpc {* Lock access to Error Status/ID Regs * and error producer/consumer index */ -struct mutex rpc_mutex;/* -* only one thread could do -* recovery on the same -* root port hierarchy -*/ };struct aer_broadcast_data {drivers/pci/pcie/aer/aerdrv_core.c +0
-0
drivers/pci/pcie/aer/aerdrv_core.c void aer_isr(struct work_struct *work) · line 802
Quoted 9 lines
drivers/pci/pcie/aer/aerdrv_core.c +2
-2@@ -796,10 +796,10 @@ void aer_isr(struct work_struct *work) struct aer_rpc *rpc = container_of(work, struct aer_rpc, dpc_handler);struct aer_err_source uninitialized_var (e_src ); -mutex_lock (&rpc->rpc_mutex); +pci_lock_rescan_remove ();while (get_e_source (rpc, &e_src))aer_isr_one_error (rpc, &e_src); -mutex_unlock (&rpc->rpc_mutex); +pci_unlock_rescan_remove ();
I think this needs to be updated after Oza's patches, doesn't it?
It looks like this would deadlock if I applied it to my current "next" branch as-is:
aer_isr
pci_lock_rescan_remove
aer_isr_one_error
aer_process_err_devices
handle_error_source
pcie_do_fatal_recovery
pci_lock_rescan_remove <-- deadlock
Quoted 5 lines
drivers/pci/pcie/aer/aerdrv_core.c +0
-0aer_release (rpc); }Signature · 1 line
2.14.3
Re: [PATCH 4/4] PCI/AER: Lock pci topology when scanning errors
Quoted 16 lines · Bjorn Helgaas
drivers/pci/pcie/aer/aerdrv_core.c void aer_isr(struct work_struct *work) · line 802
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_core.c +2
-2@@ -796,10 +796,10 @@ void aer_isr(struct work_struct *work) struct aer_rpc *rpc = container_of(work, struct aer_rpc, dpc_handler);struct aer_err_source uninitialized_var (e_src ); -mutex_lock (&rpc->rpc_mutex); +pci_lock_rescan_remove ();while (get_e_source (rpc, &e_src))aer_isr_one_error (rpc, &e_src); -mutex_unlock (&rpc->rpc_mutex); +pci_unlock_rescan_remove ();I think this needs to be updated after Oza's patches, doesn't it?
It looks like this would deadlock if I applied it to my current "next" branch as-is:
Quoted 10 lines
aer_isr
pci_lock_rescan_remove
aer_isr_one_error
aer_process_err_devices
handle_error_source
pcie_do_fatal_recovery
pci_lock_rescan_remove<-- deadlockQuoted 2 lines
drivers/pci/pcie/aer/aerdrv_core.c +0
-0aer_release (rpc); }
Yes, looks like you are right about that.
I fully intended to have this rebased on that by now, but nvme issues took way more time than I anticipated. Things appear to have calmed down on that front, and I should be able to rebase appropriately this week (famous last words...).
Re: [PATCH 4/4] PCI/AER: Lock pci topology when scanning errors
Quoted 28 lines · Keith Busch
Quoted 16 lines · Bjorn Helgaas
drivers/pci/pcie/aer/aerdrv_core.c void aer_isr(struct work_struct *work) · line 802
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_core.c +2
-2@@ -796,10 +796,10 @@ void aer_isr(struct work_struct *work) struct aer_rpc *rpc = container_of(work, struct aer_rpc, dpc_handler);struct aer_err_source uninitialized_var (e_src ); -mutex_lock (&rpc->rpc_mutex); +pci_lock_rescan_remove ();while (get_e_source (rpc, &e_src))aer_isr_one_error (rpc, &e_src); -mutex_unlock (&rpc->rpc_mutex); +pci_unlock_rescan_remove ();I think this needs to be updated after Oza's patches, doesn't it?
It looks like this would deadlock if I applied it to my current "next" branch as-is:
Quoted 10 lines
aer_isr
pci_lock_rescan_remove
aer_isr_one_error
aer_process_err_devices
handle_error_source
pcie_do_fatal_recovery
pci_lock_rescan_remove<-- deadlockQuoted 2 lines
drivers/pci/pcie/aer/aerdrv_core.c +0
-0aer_release (rpc); }
Quoted 6 lines
Yes, looks like you are right about that.
I fully intended to have this rebased on that by now, but nvme issues took way more time than I anticipated. Things appear to have calmed down on that front, and I should be able to rebase appropriately this week (famous last words...).
No worries, it's doubtful that I can still squeeze it into v4.18, so I think it's better if we target this for v4.19.
I have some questions unrelated to the rebase:
- What does the use-after-free look like to a user? Panic, corruption, etc? It's nice if we have breadcrumbs that connect a symptom to the fix.
- I'm not super comfortable with the AER tree traversal in find_source_device(). Obviously this has always been there and is not your issue. But it's dubious when a driver for device X also peeks at devices Y, Z, etc. That always leads to locking issues.
- I'm not clear on how pci_bus_sem and pci_rescan_remove_lock should be used. pci_bus_sem is acquired by pci_walk_bus() and a few other PCI core paths. pci_rescan_remove_lock is acquired (via pci_lock_rescan_remove()) by hotplug drivers and a few other add/remove/rescan paths.
Most users of pci_walk_bus() do not use pci_lock_rescan_remove(), so it's not clear to me how to decide whether they *all* should, or why this AER path is different from the ones that don't need to.
Bjorn
Quoted 5 lines · Keith Busch
AER error handling walks the PCI topology below a root port, saving pointers of the
pci_devstructs affected by the error along the way.At the same time, the pcie hotplug driver could be freeing those very same structures, causing the AER driver to reference freed memory.
I also have met such issue. The details see the below link.
https://www.spinics.net/lists/linux-pci/msg70614.html
It seems do reset_link() will trigger hotplug driver to remove/rescan device when met AER fatal error.do_recovery()
--- reset_link()
--- pci_reset_secondary_bus() //then trigger link down/up.
So if the root port support hotplug, it will remove/rescan device. I still have a question, since AER driver have already done recovery, it seems no need to trigger hotplug to remove and rescan the device.
Thanks,
Dongdong
Quoted 18 lines
This series fixes this by synchroniziing the aer driver with the pci hotplug driver during. The final patch is the one that ultimately provides the locking by having AER lock the same pci lock rescan/remove mutex as the pciehp driver. The first three patches are prepping to make it safe for the aer bottom half handler to hold that lock.
Keith Busch (4):
PCI/AER: Remove unused parameters PCI/AER: Replace structpcie_devicewithpci_devPCI/AER: Reference count aer structures PCI/AER: Lock pci topology when scanning errorsdrivers/pci/pcie/aer/aerdrv.c | 28 +++++++++++++++++++++------- drivers/pci/pcie/aer/aerdrv.h | 9 +++------ drivers/pci/pcie/aer/aerdrv_core.c | 38 +++++++++++++++++--------------------- 3 files changed, 41 insertions(+), 34 deletions(-)
From: Keith Busch [mailto:[email protected]]=20
Quoted 1 line
AER error handling walks the PCI topology below a root port, saving point=
ers of the pci_dev structs affected by the error along the way.
Hi Keith,
I've been trying to do an ABA test to confirm that your change eliminates t= he use-after-free issue we've seen. The race seems to be quite elusive, so = I can't reliably reproduce it. Your changes have not been forgotten; I have=
them staged for further testing.
Alex
Quoted 5 lines · On Thu, Apr 12, 2018 at 05:06:05PM +0000, [email protected] wrote:
From: Keith Busch [mailto:[email protected]]
Quoted 2 lines
AER error handling walks the PCI topology below a root port, saving pointers of the
pci_devstructs affected by the error along the way.
Quoted 8 lines
Hi Keith,
I've been trying to do an ABA test to confirm that your change eliminates the use-after-free issue we've seen. The race seems to be quite elusive, so I can't reliably reproduce it. Your changes have not been forgotten; I have them staged for further testing.
Alex
No worries. This is essentially just a "safe" version of the patch that you had tested, so if that one was successful, this one should be too.
Quoted 9 lines · On Thu, Apr 12, 2018 at 05:06:05PM +0000, [email protected] wrote:
From: Keith Busch [mailto:[email protected]]
Quoted 1 line
AER error handling walks the PCI topology below a root port, saving pointers of the
pci_devstructs affected by the error along the way.Hi Keith,
I've been trying to do an ABA test to confirm that your change eliminates the use-after-free issue we've seen. The race seems to be quite elusive, so I can't reliably reproduce it. Your changes have not been forgotten; I have them staged for further testing.
Alex
If you need help triggering the race you can add a sleep/microsleep here:
aer_isr_one_error() between the find_source_device and process err device:
sbauer@sbauer-Z170X-UD5:~/nvme_code/upstream_jens/linux-block$ git diff drivers/pci/pcie/aer/aerdrv_core.c
drivers/pci/pcie/aer/aerdrv_core.c +7 -2
@@ -22,6 +22,7 @@#include <linux/delay.h> #include <linux/slab.h> #include <linux/kfifo.h> +#include <linux/delay.h> #include "aerdrv.h" #define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVCTL_NFERE | \
@@ -740,8 +741,10 @@ static void aer_isr_one_error(struct pcie_device *p_device,aer_print_port_info (p_device->port, e_info); -if (find_source_device (p_device->port, e_info)) +if (find_source_device (p_device->port, e_info)) { +msleep (350 );aer_process_err_devices (p_device, e_info); + } }if (e_src->status &PCI_ERR_ROOT_UNCOR_RCV ) {
@@ -759,8 +762,10 @@ static void aer_isr_one_error(struct pcie_device *p_device,aer_print_port_info (p_device->port, e_info); -if (find_source_device (p_device->port, e_info)) +if (find_source_device (p_device->port, e_info)) { +msleep (350 );aer_process_err_devices (p_device, e_info); + } } }
I got the cold chills when I realized you called for a delay of 350ms. It's=
because 350ms is around the delay I've observed to be caused by FFS.
First run KASANed with the extra delay, so hopefully, I'll have more cement=
test results by EOB today.
Alex
-----Original Message-----
From: Scott Bauer [mailto:[email protected]]=20
Sent: Thursday, April 12, 2018 11:47 AM
To: Gagniuc, Alexandru - Dell Team
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 0/4] PCI/AER: Use-after-free fix
Quoted 3 lines · On Thu, Apr 12, 2018 at 05:06:05PM +0000, [email protected] wrote:
From: Keith Busch [mailto:[email protected]]
=20Quoted 1 line
AER error handling walks the PCI topology below a root port, saving poi=
nters of the pci_dev structs affected by the error along the way.
Quoted 4 lines
=20
Hi Keith,
=20
I've been trying to do an ABA test to confirm that your change eliminates=
the use-after-free issue we've seen. The race seems to be quite elusive, s=
o I can't reliably reproduce it. Your changes have not been forgotten; I ha= ve them staged for further testing.
Quoted 2 lines
=20
Alex
If you need help triggering the race you can add a sleep/microsleep here:
aer_isr_one_error() between the find_source_device and process err device:
sbauer@sbauer-Z170X-UD5:~/nvme_code/upstream_jens/linux-block$ git diff dri= vers/pci/pcie/aer/aerdrv_core.c
drivers/pci/pcie/aer/aerd= renamed from drivers/pci/pcie/aer/aerdrv_core.c +0 -0
rv_core.c
index a4bfea52e7d4..5ca0c07b1d05 100644
drivers/pci/pcie/aer/aerdrv_core.c +1 -0
@@ -22,6 +22,7 @@#include <linux/delay.h> #include <linux/slab.h> #include <linux/kfifo.h> +#include <linux/delay.h> #include "aerdrv.h"
=20
#define PCI_EXP_AER_FLAGS (PCI_EXP_DEVCTL_CERE | PCI_EXP_DEVC=TL_NFERE | \
@@ -740,8 +741,10 @@ static void aer_isr_one_error(struct pcie_device *p_de= vice,
=20
aer_print_port_info(p_device->port, e_info);
=20
- if (find_source_device(p_device->port, e_info))
+ if (find_source_device(p_device->port, e_info)) {
+ msleep(350);
aer_process_err_devices(p_device, e_info);
+ }
}
=20
if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) { @@ -759,8 +762,10 @@ =
static void aer_isr_one_error(struct pcie_device *p_device,
=20
aer_print_port_info(p_device->port, e_info);
=20
- if (find_source_device(p_device->port, e_info))
+ if (find_source_device(p_device->port, e_info)) {
+ msleep(350);
aer_process_err_devices(p_device, e_info);
+ }
}}
[email protected]
Tested-by Alexandru GagniucFrom: Scott Bauer [mailto:[email protected]]=20
Quoted 3 lines · On Thu, Apr 12, 2018 at 05:06:05PM +0000, [email protected] wrote:
From: Keith Busch [mailto:[email protected]]
=20Quoted 1 line
AER error handling walks the PCI topology below a root port, saving poi=
nters of the pci_dev structs affected by the error along the way.
Quoted 4 lines
=20
Hi Keith,
=20
I've been trying to do an ABA test to confirm that your change eliminates=
the use-after-free issue we've seen. The race seems to be quite elusive, s=
o I can't reliably reproduce it. Your changes have not been forgotten; I ha= ve them staged for further testing.
Quoted 5 lines
=20
AlexIf you need help triggering the race you can add a sleep/microsleep here:
Bjorn Helgaas
AppliedQuoted 18 lines · Keith Busch
AER error handling walks the PCI topology below a root port, saving pointers of the
pci_devstructs affected by the error along the way.At the same time, the pcie hotplug driver could be freeing those very same structures, causing the AER driver to reference freed memory.
This series fixes this by synchroniziing the aer driver with the pci hotplug driver during. The final patch is the one that ultimately provides the locking by having AER lock the same pci lock rescan/remove mutex as the pciehp driver. The first three patches are prepping to make it safe for the aer bottom half handler to hold that lock.
Keith Busch (4):
PCI/AER: Remove unused parameters PCI/AER: Replace structpcie_devicewithpci_devPCI/AER: Reference count aer structures PCI/AER: Lock pci topology when scanning errors
Quoted 4 lines
drivers/pci/pcie/aer/aerdrv.c | 28 +++++++++++++++++++++------- drivers/pci/pcie/aer/aerdrv.h | 9 +++------ drivers/pci/pcie/aer/aerdrv_core.c | 38 +++++++++++++++++--------------------- 3 files changed, 41 insertions(+), 34 deletions(-)
I applied the first two to pci/aer for v4.18, thanks!
I think the last two might need some rework to adapt after Oza's patches.