avoid null pointer rereference during FLR
Christoph Hellwig
maintainerHi all,
Rakesh reported a bug where a FLR can trivially crash his system. The reason for that is that NVMe unbinds the driver from the PCI device on an unrecoverable error, and that races with the reset_notify method.
This is fairly easily fixable by taking the device lock for a slightly longer period. Note that the other PCI error handling methods actually have the same issue, but with them not taking the lock yet and me having no good way to reproducibly call them I'm a little reluctant to touch them, but it would be great if we could fix those issues as well.
Christoph Hellwig
maintainerPATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
Without this ->notify_reset instance may race with ->remove calls, which can be easily triggered in NVMe.
--- drivers/pci/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pci/pci.c +3 -1
@@ -4275,11 +4275,13 @@ int pci_reset_function(struct pci_dev *dev)if (rc)return rc; +pci_dev_lock (dev);pci_dev_save_and_disable (dev); - rc =pci_dev_reset (dev,0 ); + rc =__pci_dev_reset (dev,0 );pci_dev_restore (dev); +pci_dev_unlock (dev);return rc; }
Signature · 1 line
2.11.0
PATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
Hi,
Quoted 3 lines · Christoph Hellwig
Without this ->notify_reset instance may race with ->remove calls, which can be easily triggered in NVMe.
Any input on this sometime before next -rc release would be great ?
Quoted 29 lines
Reported-by Rakesh Pandit <rakesh at tuxera.com>Tested-by Rakesh Pandit <rakesh at tuxera.com>Signed-off-by Christoph Hellwig <hch at lst.de>--- drivers/pci/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/pci/pci.c +3
-1@@ -4275,11 +4275,13 @@ int pci_reset_function(struct pci_dev *dev)if (rc)return rc; +pci_dev_lock (dev);pci_dev_save_and_disable (dev); - rc =pci_dev_reset (dev,0 ); + rc =__pci_dev_reset (dev,0 );pci_dev_restore (dev); +pci_dev_unlock (dev);return rc; }Signature · 1 line
2.11.0
PATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
In reply to Christoph Hellwig[+cc Alex]
Quoted 1 line · Christoph Hellwig
Without this ->notify_reset instance may race with ->remove calls,
Do you mean the .reset_notify() method in struct pci_error_handlers? I don't see a "notify_reset" symbol.
Can you elaborate on exactly how this race happens? I'm trying to figure out whether this is also a problem or potential problem with other reset paths like pci_try_reset_function(), pci_reset_bus(), pci_try_reset_bus(), pci_reset_slot(), and pci_try_reset_slot().
What does the race look like when it happens? Oops, panic, etc?
Can this also be triggered via the sysfs "reset" file?
Quoted 31 lines
which can be easily triggered in NVMe.
Reported-by Rakesh Pandit <rakesh at tuxera.com>Tested-by Rakesh Pandit <rakesh at tuxera.com>Signed-off-by Christoph Hellwig <hch at lst.de>--- drivers/pci/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/pci/pci.c +3
-1@@ -4275,11 +4275,13 @@ int pci_reset_function(struct pci_dev *dev)if (rc)return rc; +pci_dev_lock (dev);pci_dev_save_and_disable (dev); - rc =pci_dev_reset (dev,0 ); + rc =__pci_dev_reset (dev,0 );pci_dev_restore (dev); +pci_dev_unlock (dev);return rc; }Signature · 1 line
2.11.0
Christoph Hellwig
maintainerPATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
Quoted 7 lines · Bjorn Helgaas
[+cc Alex]
Quoted 1 line · Christoph Hellwig
Without this ->notify_reset instance may race with ->remove calls,
Do you mean the .reset_notify() method in struct
pci_error_handlers? I don't see a "notify_reset" symbol.
Yes.
Quoted 8 lines
Can you elaborate on exactly how this race happens? I'm trying to figure out whether this is also a problem or potential problem with other reset paths like
pci_try_reset_function(),pci_reset_bus(),pci_try_reset_bus(),pci_reset_slot(), andpci_try_reset_slot().What does the race look like when it happens? Oops, panic, etc?
Can this also be triggered via the sysfs "reset" file?
Here is the original bug report:
http://lists.infradead.org/pipermail/linux-nvme/2017-May/010345.html
the issue is triggered through sysfs.
PATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
Quoted 18 lines · Christoph Hellwig
Quoted 7 lines · Bjorn Helgaas
[+cc Alex]
Quoted 1 line · Christoph Hellwig
Without this ->notify_reset instance may race with ->remove calls,
Do you mean the .reset_notify() method in struct
pci_error_handlers? I don't see a "notify_reset" symbol.Yes.
Quoted 7 lines
Can you elaborate on exactly how this race happens? I'm trying to figure out whether this is also a problem or potential problem with other reset paths like
pci_try_reset_function(),pci_reset_bus(),pci_try_reset_bus(),pci_reset_slot(), andpci_try_reset_slot().What does the race look like when it happens? Oops, panic, etc?
Quoted 7 lines
Quoted 1 line
Can this also be triggered via the sysfs "reset" file?
Here is the original bug report:
http://lists.infradead.org/pipermail/linux-nvme/2017-May/010345.html
the issue is triggered through sysfs.
Thanks.
Do you have any thoughts on whether the paths I mentioned above are also susceptible?
I don't want to apply a patch that fixes one issue while leaving similar ones unfixed.
Bjorn
Christoph Hellwig
maintainerPATCH 1/1 PCI: ensure the PCI device is locked over ->reset_notify calls
Quoted 2 lines · Bjorn Helgaas
Do you have any thoughts on whether the paths I mentioned above are also susceptible?
At least in theory ever method call that doesn't have the device locked is susceptible, so yes.