[PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

Hi Bjorn,
Thank you very much to review the V1 patch. I reworked the patch as you have suggested. Your suggestion is indeed better and cleaner. Would you please have a look for me?

FYI, I did not clear the device structure in aer_isr_one_error() just to avoid over complicated the fix.

Also, thank you very much to read thru the caller of pcie_do_fatal_recovery() to ensure no one is using the device structure. Really appricated your extra effort.

Thank you,
Thomas

[PATCH V2, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

When an fatal error is recevied by a non-bridge device,
the device is removed from the pci bus and the device structure
is freed by pci_stop_and_remove_bus_device(). The freed device
structure is used in the subsequence pci_info() to printout the
message. It causes a corrupt printout. If slub_debug=FZP is used,
it will cause following protection fault after a fatal error is
received.

general protection fault: 0000 [#1] SMP PTI
CPU: 104 PID: 1077 Comm: kworker/104:1 Not tainted 4.18.0-rc1ttai #5
Hardware name: Oracle Corporation ORACLE SERVER X5-4/ASSY,MB WITH TRAY,
BIOS 36030500 11/16/2016
Workqueue: events aer_isr
 RIP: 0010:__dev_printk+0x2e/0x90
 Code: 00 55 49 89 d1 48 89 e5 53 48 89 fb 48 83 ec 18 48 85 f6
 74 5f 4c 8b 46 50 4d 85 c0 74 2b 48 8b 86 88 00 00 00 48 85 c0
 74 25 <48> 8b 08 0f be 7b 01 48 c7 c2 83 d4 71 99 31 c0 83 ef
 30 e8 4a ff
 RSP: 0018:ffffb6b88fa57cf8 EFLAGS: 00010202
 RAX: 6b6b6b6b6b6b6b6b RBX: ffffffff996ba720 RCX: 0000000000000000
 RDX: ffffb6b88fa57d28 RSI: ffff8c4d7af94128 RDI: ffffffff996ba720
 RBP: ffffb6b88fa57d18 R08: 6b6b6b6b6b6b6b6b R09: ffffb6b88fa57d28
 R10: ffffffff99baca80 R11: 0000000000000000 R12: ffff8c4d7ae95990
 R13: ffff8c2d7a840008 R14: ffff8c4d7af94088 R15: ffff8c4d7af90008
 FS:  0000000000000000(0000) GS:ffff8c2d7fc00000(0000)
knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f22c0839000 CR3: 000000136bc0a001 CR4: 00000000001606e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  ? pci_bus_add_device+0x4f/0xa0
  _dev_info+0x6c/0x90
  pcie_do_fatal_recovery+0x1d5/0x230
  aer_isr+0x3e5/0x950
  ? add_timer_on+0xcc/0x160
  process_one_work+0x168/0x370
  worker_thread+0x4f/0x3d0
  kthread+0x105/0x140
  ? max_active_store+0x80/0x80
  ? kthread_bind+0x20/0x20
  ret_from_fork+0x35/0x40

To fix this issue, pci_dev_get and pci_dev_put is placed at the
entry and exit of the function. This way the device structure is
free only at the end of the function if needed.

Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
---
 drivers/pci/pcie/err.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index f7ce0cb..e5decd8 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -288,6 +288,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 	struct pci_dev *pdev, *temp;
 	pci_ers_result_t result;
 
+	pci_dev_get(dev);
 	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
 		udev = dev;
 	else
@@ -329,6 +330,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 	}
 
 	pci_unlock_rescan_remove();
+	pci_dev_put(dev);
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Bjorn Helgaas]

[+cc Keith for DPC question]

On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
> Hi Bjorn,
> Thank you very much to review the V1 patch. I reworked the patch as
> you have suggested. Your suggestion is indeed better and cleaner.
> Would you please have a look for me?
> 
> FYI, I did not clear the device structure in aer_isr_one_error()
> just to avoid over complicated the fix.

I think that was a bad idea anyway.  I think the problem there is that
add_error_device() copies the pci_dev pointer without taking a
reference on it:

  aer_isr
    get_e_source              # dequeue from rpc->e_sources[]
      *e_src = rpc->e_sources[x]
    aer_isr_one_error
      pdev = rpc->rpd         # pdev = Root Port pci_dev
      find_source_device(pdev)
	find_device_iter
	  add_error_device
	    e_info->dev[i] = dev   # <-- save pci_dev pointer
      aer_process_err_devices
	handle_error_source(e_info->dev[i])
	  pcie_do_fatal_recovery

At this point, we have pci_dev pointers in e_info->dev[] that are
potentially stale.  Right now nobody uses them, but I think it's
sloppy that we still have them at all.

I think a better way to fix this would be to call pci_dev_get() in
add_error_device() when we save the pointer, and then do the
corresponding pci_dev_put() in aer_isr_one_error(), after we return
from aer_process_err_devices().

This could be done with a little helper function that iterates through
e_info->dev[i], calls pci_dev_put() for each, and clears
e_info->error_dev_num (which could then be removed from
find_source_device()).

I think this would fix the problem you're seeing, so we wouldn't need
the change in pcie_do_fatal_recovery().

However, I think we're also slightly exposed in dpc_work(), in basically
the same (possibly harmless) way.

  dpc_irq
    schedule_work(&dpc->work)
  ...
  dpc_work
    pdev = dpc->dev->port
    pcie_do_fatal_recovery(pdev)

pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
holding onto a pointer (which it never uses again).

The DPC driver should be holding a reference to pdev (through some black
magic I don't understand), but that would be released when pdev is removed,
and I don't know what ensures that dpc_work() runs before that release.

Bjorn

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Keith Busch]

On Thu, Jul 12, 2018 at 04:51:51PM -0500, Bjorn Helgaas wrote:
> However, I think we're also slightly exposed in dpc_work(), in basically
> the same (possibly harmless) way.
> 
>   dpc_irq
>     schedule_work(&dpc->work)
>   ...
>   dpc_work
>     pdev = dpc->dev->port
>     pcie_do_fatal_recovery(pdev)
> 
> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
> holding onto a pointer (which it never uses again).
> 
> The DPC driver should be holding a reference to pdev (through some black
> magic I don't understand), but that would be released when pdev is removed,
> and I don't know what ensures that dpc_work() runs before that release.
> 
> Bjorn

Yep, you're right on that point. There's different ways we can fix
that. The most recent one I proposed was to replace the scheduled work
with the threaded irq[1]. That should make it safe since the lifetime of
when bottom half can be executed is tied to the lifetime of the device
that registered it.

 1. https://patchwork.kernel.org/patch/10478755/

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

On 07/12/2018 05:57 PM, Keith Busch wrote:
> On Thu, Jul 12, 2018 at 04:51:51PM -0500, Bjorn Helgaas wrote:
>> However, I think we're also slightly exposed in dpc_work(), in basically
>> the same (possibly harmless) way.
>>
>>    dpc_irq
>>      schedule_work(&dpc->work)
>>    ...
>>    dpc_work
>>      pdev = dpc->dev->port
>>      pcie_do_fatal_recovery(pdev)
>>
>> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
>> holding onto a pointer (which it never uses again).
>>
>> The DPC driver should be holding a reference to pdev (through some black
>> magic I don't understand), but that would be released when pdev is removed,
>> and I don't know what ensures that dpc_work() runs before that release.
>>
>> Bjorn
> 
> Yep, you're right on that point. There's different ways we can fix
> that. The most recent one I proposed was to replace the scheduled work
> with the threaded irq[1]. That should make it safe since the lifetime of
> when bottom half can be executed is tied to the lifetime of the device
> that registered it.
> 
>   1. https://patchwork.kernel.org/patch/10478755/

Hi Bjorn, Keith and Poza,
I like the idea of using threaded irq if it can hold the device until 
the bottom half finished. It makes the AER and DPC driver codes more 
consistent. One problem I hit when changing the AER to threaded irq is 
that the error injection module aer_inject won't work anymore because it 
call the aer_irq directly and didn't go thru the threaded_irq path. I 
would have to fix that also. Will keep you posted.

Cheers,
Thomas

> 

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Bjorn Helgaas]

On Thu, Jul 12, 2018 at 03:57:01PM -0600, Keith Busch wrote:
> On Thu, Jul 12, 2018 at 04:51:51PM -0500, Bjorn Helgaas wrote:
> > However, I think we're also slightly exposed in dpc_work(), in basically
> > the same (possibly harmless) way.
> > 
> >   dpc_irq
> >     schedule_work(&dpc->work)
> >   ...
> >   dpc_work
> >     pdev = dpc->dev->port
> >     pcie_do_fatal_recovery(pdev)
> > 
> > pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
> > holding onto a pointer (which it never uses again).
> > 
> > The DPC driver should be holding a reference to pdev (through some black
> > magic I don't understand), but that would be released when pdev is removed,
> > and I don't know what ensures that dpc_work() runs before that release.
> > 
> > Bjorn
> 
> Yep, you're right on that point. There's different ways we can fix
> that. The most recent one I proposed was to replace the scheduled work
> with the threaded irq[1]. That should make it safe since the lifetime of
> when bottom half can be executed is tied to the lifetime of the device
> that registered it.
> 
>  1. https://patchwork.kernel.org/patch/10478755/

This one is headed for v4.19 (it's on pci/dpc right now).  I don't
think anybody will trip over this with DPC, so I don't think we need
to get this in v4.18 (let me know if you think otherwise).

But of course it doesn't fix the crash Thomas is seeing, because
that's in the AER path, not the DPC path.  I think you're working on
an AER fix involving threaded IRQs, right, Thomas?  Just want to make
sure I haven't missed something.

Bjorn

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

On 07/18/2018 05:16 PM, Bjorn Helgaas wrote:
> On Thu, Jul 12, 2018 at 03:57:01PM -0600, Keith Busch wrote:
>> On Thu, Jul 12, 2018 at 04:51:51PM -0500, Bjorn Helgaas wrote:
>>> However, I think we're also slightly exposed in dpc_work(), in basically
>>> the same (possibly harmless) way.
>>>
>>>    dpc_irq
>>>      schedule_work(&dpc->work)
>>>    ...
>>>    dpc_work
>>>      pdev = dpc->dev->port
>>>      pcie_do_fatal_recovery(pdev)
>>>
>>> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
>>> holding onto a pointer (which it never uses again).
>>>
>>> The DPC driver should be holding a reference to pdev (through some black
>>> magic I don't understand), but that would be released when pdev is removed,
>>> and I don't know what ensures that dpc_work() runs before that release.
>>>
>>> Bjorn
>>
>> Yep, you're right on that point. There's different ways we can fix
>> that. The most recent one I proposed was to replace the scheduled work
>> with the threaded irq[1]. That should make it safe since the lifetime of
>> when bottom half can be executed is tied to the lifetime of the device
>> that registered it.
>>
>>   1. https://patchwork.kernel.org/patch/10478755/
> 
> This one is headed for v4.19 (it's on pci/dpc right now).  I don't
> think anybody will trip over this with DPC, so I don't think we need
> to get this in v4.18 (let me know if you think otherwise).
> 
> But of course it doesn't fix the crash Thomas is seeing, because
> that's in the AER path, not the DPC path.  I think you're working on
> an AER fix involving threaded IRQs, right, Thomas?  Just want to make
> sure I haven't missed something.

Yes, Bjorn. I am working on the aer path. I followed Keith's patch and 
use threaded_irq in the aer. Even with the threaded_irq, the device got 
freed in the pcie_do_fatal_receovery(). So I am suggesting to use your 
idea, that is, have a helper functions to get/put the device.


> 
> Bjorn
> 

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Bjorn Helgaas]

On Wed, Jul 18, 2018 at 05:43:03PM -0400, Thomas Tai wrote:
> On 07/18/2018 05:16 PM, Bjorn Helgaas wrote:
> > On Thu, Jul 12, 2018 at 03:57:01PM -0600, Keith Busch wrote:
> > > On Thu, Jul 12, 2018 at 04:51:51PM -0500, Bjorn Helgaas wrote:
> > > > However, I think we're also slightly exposed in dpc_work(), in basically
> > > > the same (possibly harmless) way.
> > > > 
> > > >    dpc_irq
> > > >      schedule_work(&dpc->work)
> > > >    ...
> > > >    dpc_work
> > > >      pdev = dpc->dev->port
> > > >      pcie_do_fatal_recovery(pdev)
> > > > 
> > > > pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
> > > > holding onto a pointer (which it never uses again).
> > > > 
> > > > The DPC driver should be holding a reference to pdev (through some black
> > > > magic I don't understand), but that would be released when pdev is removed,
> > > > and I don't know what ensures that dpc_work() runs before that release.
> > > > 
> > > > Bjorn
> > > 
> > > Yep, you're right on that point. There's different ways we can fix
> > > that. The most recent one I proposed was to replace the scheduled work
> > > with the threaded irq[1]. That should make it safe since the lifetime of
> > > when bottom half can be executed is tied to the lifetime of the device
> > > that registered it.
> > > 
> > >   1. https://patchwork.kernel.org/patch/10478755/
> > 
> > This one is headed for v4.19 (it's on pci/dpc right now).  I don't
> > think anybody will trip over this with DPC, so I don't think we need
> > to get this in v4.18 (let me know if you think otherwise).
> > 
> > But of course it doesn't fix the crash Thomas is seeing, because
> > that's in the AER path, not the DPC path.  I think you're working on
> > an AER fix involving threaded IRQs, right, Thomas?  Just want to make
> > sure I haven't missed something.
> 
> Yes, Bjorn. I am working on the aer path. I followed Keith's patch and use
> threaded_irq in the aer. Even with the threaded_irq, the device got freed in
> the pcie_do_fatal_receovery(). So I am suggesting to use your idea, that is,
> have a helper functions to get/put the device.

Hmmm, the fact that it happens even with threaded IRQs is concerning
because it suggests that we don't understand the threaded IRQ
synchronization completely (I know for sure *I* don't!), and Keith's patch
above may not solve the problem even for DPC.

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[poza]

On 2018-07-13 03:21, Bjorn Helgaas wrote:
> [+cc Keith for DPC question]
> 
> On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
>> Hi Bjorn,
>> Thank you very much to review the V1 patch. I reworked the patch as
>> you have suggested. Your suggestion is indeed better and cleaner.
>> Would you please have a look for me?
>> 
>> FYI, I did not clear the device structure in aer_isr_one_error()
>> just to avoid over complicated the fix.
> 
> I think that was a bad idea anyway.  I think the problem there is that
> add_error_device() copies the pci_dev pointer without taking a
> reference on it:
> 
>   aer_isr
>     get_e_source              # dequeue from rpc->e_sources[]
>       *e_src = rpc->e_sources[x]
>     aer_isr_one_error
>       pdev = rpc->rpd         # pdev = Root Port pci_dev
>       find_source_device(pdev)
> 	find_device_iter
> 	  add_error_device
> 	    e_info->dev[i] = dev   # <-- save pci_dev pointer
>       aer_process_err_devices
> 	handle_error_source(e_info->dev[i])
> 	  pcie_do_fatal_recovery
> 
> At this point, we have pci_dev pointers in e_info->dev[] that are
> potentially stale.  Right now nobody uses them, but I think it's
> sloppy that we still have them at all.
> 
> I think a better way to fix this would be to call pci_dev_get() in
> add_error_device() when we save the pointer, and then do the
> corresponding pci_dev_put() in aer_isr_one_error(), after we return
> from aer_process_err_devices().
> 
> This could be done with a little helper function that iterates through
> e_info->dev[i], calls pci_dev_put() for each, and clears
> e_info->error_dev_num (which could then be removed from
> find_source_device()).
> 
> I think this would fix the problem you're seeing, so we wouldn't need
> the change in pcie_do_fatal_recovery().
> 
> However, I think we're also slightly exposed in dpc_work(), in 
> basically
> the same (possibly harmless) way.
> 
>   dpc_irq
>     schedule_work(&dpc->work)
>   ...
>   dpc_work
>     pdev = dpc->dev->port
>     pcie_do_fatal_recovery(pdev)
> 
> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is 
> still
> holding onto a pointer (which it never uses again).
> 
> The DPC driver should be holding a reference to pdev (through some 
> black
> magic I don't understand), but that would be released when pdev is 
> removed,
> and I don't know what ensures that dpc_work() runs before that release.
> 

Is not that dpc_work() is calling pcie_do_fatal_recovery() which in-turn 
is removing pdev.
I do not see DPC is using any more reference beyond that.
So, isnt it safe to assume that no matter when the dpc_work() run, the 
pdev it reference is always intact.
this is with the fact, that both AER and DPC can not trigger 
simultaneously, and and hence no race for removal.
who else would remove that pdev other than AER and DPC ?

With Keith's patch, are not we just improving the latency with threaded 
irq instead of workqueue ?

the potential problem is another interrupt arriving while we have 
acknowledged it in dpc_reset_link() and we are about to remove the 
devices.
The acknowledgement of DPC status trigger and re-enabling interrupt can 
be done in
dpc_work() or threaded_handler() after we are done with 
pcie_do_fatal_recovery()

dpc_reset_link()
{
         following can be moved after pcie_do_fatal_recovery() ??
	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
			      PCI_EXP_DPC_STATUS_TRIGGER);

	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
			      ctl | PCI_EXP_DPC_CTL_INT_EN);
}

and probably following also one also can be done after recovery.

pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
			      PCI_EXP_DPC_STATUS_INTERRUPT);

I am not sure if this was the concern or I did talk about something else 
all together !

> Bjorn

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Keith Busch]

On Mon, Jul 16, 2018 at 12:25:07AM +0530, poza@codeaurora.org wrote:
> On 2018-07-13 03:21, Bjorn Helgaas wrote:
> > On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
> > > Hi Bjorn,
> > > Thank you very much to review the V1 patch. I reworked the patch as
> > > you have suggested. Your suggestion is indeed better and cleaner.
> > > Would you please have a look for me?
> > > 
> > > FYI, I did not clear the device structure in aer_isr_one_error()
> > > just to avoid over complicated the fix.
> > 
> > I think that was a bad idea anyway.  I think the problem there is that
> > add_error_device() copies the pci_dev pointer without taking a
> > reference on it:
> > 
> >   aer_isr
> >     get_e_source              # dequeue from rpc->e_sources[]
> >       *e_src = rpc->e_sources[x]
> >     aer_isr_one_error
> >       pdev = rpc->rpd         # pdev = Root Port pci_dev
> >       find_source_device(pdev)
> > 	find_device_iter
> > 	  add_error_device
> > 	    e_info->dev[i] = dev   # <-- save pci_dev pointer
> >       aer_process_err_devices
> > 	handle_error_source(e_info->dev[i])
> > 	  pcie_do_fatal_recovery
> > 
> > At this point, we have pci_dev pointers in e_info->dev[] that are
> > potentially stale.  Right now nobody uses them, but I think it's
> > sloppy that we still have them at all.
> > 
> > I think a better way to fix this would be to call pci_dev_get() in
> > add_error_device() when we save the pointer, and then do the
> > corresponding pci_dev_put() in aer_isr_one_error(), after we return
> > from aer_process_err_devices().
> > 
> > This could be done with a little helper function that iterates through
> > e_info->dev[i], calls pci_dev_put() for each, and clears
> > e_info->error_dev_num (which could then be removed from
> > find_source_device()).
> > 
> > I think this would fix the problem you're seeing, so we wouldn't need
> > the change in pcie_do_fatal_recovery().
> > 
> > However, I think we're also slightly exposed in dpc_work(), in basically
> > the same (possibly harmless) way.
> > 
> >   dpc_irq
> >     schedule_work(&dpc->work)
> >   ...
> >   dpc_work
> >     pdev = dpc->dev->port
> >     pcie_do_fatal_recovery(pdev)
> > 
> > pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
> > holding onto a pointer (which it never uses again).
> > 
> > The DPC driver should be holding a reference to pdev (through some black
> > magic I don't understand), but that would be released when pdev is
> > removed,
> > and I don't know what ensures that dpc_work() runs before that release.
> > 
> 
> Is not that dpc_work() is calling pcie_do_fatal_recovery() which in-turn is
> removing pdev.
> I do not see DPC is using any more reference beyond that.
> So, isnt it safe to assume that no matter when the dpc_work() run, the pdev
> it reference is always intact.
> this is with the fact, that both AER and DPC can not trigger simultaneously,
> and and hence no race for removal.
> who else would remove that pdev other than AER and DPC ?

It's not a problem for the devices below the contained port. It's more
about the port itself, which may happen if you hot-remove the switch
that has DPC services: nothing coordinates that switch's removal with the
dpc_work so the dpc work could be referencing a pdev that pciehp removed
earlier, for example. Removing switches currently is not very common as
far as I know, but there are definitely users that want that ability.

> With Keith's patch, are not we just improving the latency with threaded irq
> instead of workqueue ?

It's a little more than that. The irq subsystem will sync with the
threaded irq before allowing teardown of the device to complete, so that
should ensure we don't have a use-after-free in the dpc bottom half.
 
> the potential problem is another interrupt arriving while we have
> acknowledged it in dpc_reset_link() and we are about to remove the devices.
> The acknowledgement of DPC status trigger and re-enabling interrupt can be
> done in
> dpc_work() or threaded_handler() after we are done with
> pcie_do_fatal_recovery()
> 
> dpc_reset_link()
> {
>         following can be moved after pcie_do_fatal_recovery() ??
> 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> 			      PCI_EXP_DPC_STATUS_TRIGGER);
> 
> 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
> 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
> 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
> }
> 
> and probably following also one also can be done after recovery.
> 
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> 			      PCI_EXP_DPC_STATUS_INTERRUPT);
> 
> I am not sure if this was the concern or I did talk about something else all
> together !

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

On 07/15/2018 02:55 PM, poza@codeaurora.org wrote:
> On 2018-07-13 03:21, Bjorn Helgaas wrote:
>> [+cc Keith for DPC question]
>>
>> On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
>>> Hi Bjorn,
>>> Thank you very much to review the V1 patch. I reworked the patch as
>>> you have suggested. Your suggestion is indeed better and cleaner.
>>> Would you please have a look for me?
>>>
>>> FYI, I did not clear the device structure in aer_isr_one_error()
>>> just to avoid over complicated the fix.
>>
>> I think that was a bad idea anyway.  I think the problem there is that
>> add_error_device() copies the pci_dev pointer without taking a
>> reference on it:
>>
>>   aer_isr
>>     get_e_source              # dequeue from rpc->e_sources[]
>>       *e_src = rpc->e_sources[x]
>>     aer_isr_one_error
>>       pdev = rpc->rpd         # pdev = Root Port pci_dev
>>       find_source_device(pdev)
>>     find_device_iter
>>       add_error_device
>>         e_info->dev[i] = dev   # <-- save pci_dev pointer
>>       aer_process_err_devices
>>     handle_error_source(e_info->dev[i])
>>       pcie_do_fatal_recovery
>>
>> At this point, we have pci_dev pointers in e_info->dev[] that are
>> potentially stale.  Right now nobody uses them, but I think it's
>> sloppy that we still have them at all.
>>
>> I think a better way to fix this would be to call pci_dev_get() in
>> add_error_device() when we save the pointer, and then do the
>> corresponding pci_dev_put() in aer_isr_one_error(), after we return
>> from aer_process_err_devices().
>>
>> This could be done with a little helper function that iterates through
>> e_info->dev[i], calls pci_dev_put() for each, and clears
>> e_info->error_dev_num (which could then be removed from
>> find_source_device()).
>>
>> I think this would fix the problem you're seeing, so we wouldn't need
>> the change in pcie_do_fatal_recovery().
>>
>> However, I think we're also slightly exposed in dpc_work(), in basically
>> the same (possibly harmless) way.
>>
>>   dpc_irq
>>     schedule_work(&dpc->work)
>>   ...
>>   dpc_work
>>     pdev = dpc->dev->port
>>     pcie_do_fatal_recovery(pdev)
>>
>> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
>> holding onto a pointer (which it never uses again).
>>
>> The DPC driver should be holding a reference to pdev (through some black
>> magic I don't understand), but that would be released when pdev is 
>> removed,
>> and I don't know what ensures that dpc_work() runs before that release.
>>
> 
> Is not that dpc_work() is calling pcie_do_fatal_recovery() which in-turn 
> is removing pdev.
> I do not see DPC is using any more reference beyond that.

Agree.

> So, isnt it safe to assume that no matter when the dpc_work() run, the 
> pdev it reference is always intact.
> this is with the fact, that both AER and DPC can not trigger 
> simultaneously, and and hence no race for removal.
> who else would remove that pdev other than AER and DPC ?

Looks like no one is removing it other than the pcie_do_fatal_recovery(pdev)

> 
> With Keith's patch, are not we just improving the latency with threaded 
> irq instead of workqueue ?

I think the most important is that the threaded_irq hold the device 
until it finished the bottom half. Although I can't see where in the 
threaded_irq code increase the device reference count. I may be missing 
some info here, either way I am trying out the threaded_irq to see if it 
fixed the issue or not.

> 
> the potential problem is another interrupt arriving while we have 
> acknowledged it in dpc_reset_link() and we are about to remove the devices.
> The acknowledgement of DPC status trigger and re-enabling interrupt can 
> be done in
> dpc_work() or threaded_handler() after we are done with 
> pcie_do_fatal_recovery()

Good point. the pcie_do_fatal_recovery() use 
pci_stop_and_remove_bus_device() to remove the device, isn't it will 
stop the device from sending out anymore interrupts? Once we remove the 
device, we probably no need to reset the device.

-Thomas

> 
> dpc_reset_link()
> {
>          following can be moved after pcie_do_fatal_recovery() ??
>      pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>                    PCI_EXP_DPC_STATUS_TRIGGER);
> 
>      pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
>      pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
>                    ctl | PCI_EXP_DPC_CTL_INT_EN);
> }
> 
> and probably following also one also can be done after recovery.
> 
> pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>                    PCI_EXP_DPC_STATUS_INTERRUPT);
> 
> I am not sure if this was the concern or I did talk about something else 
> all together !
> 
>> Bjorn

Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

Hi Bjorn, Keith,
Please have a look at the below inline comment.

On 07/12/2018 05:51 PM, Bjorn Helgaas wrote:
> [+cc Keith for DPC question]
> 
> On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
>> Hi Bjorn,
>> Thank you very much to review the V1 patch. I reworked the patch as
>> you have suggested. Your suggestion is indeed better and cleaner.
>> Would you please have a look for me?
>>
>> FYI, I did not clear the device structure in aer_isr_one_error()
>> just to avoid over complicated the fix.
> 
> I think that was a bad idea anyway.  I think the problem there is that
> add_error_device() copies the pci_dev pointer without taking a
> reference on it:
> 
>    aer_isr
>      get_e_source              # dequeue from rpc->e_sources[]
>        *e_src = rpc->e_sources[x]
>      aer_isr_one_error
>        pdev = rpc->rpd         # pdev = Root Port pci_dev
>        find_source_device(pdev)
> 	find_device_iter
> 	  add_error_device
> 	    e_info->dev[i] = dev   # <-- save pci_dev pointer
>        aer_process_err_devices
> 	handle_error_source(e_info->dev[i])
> 	  pcie_do_fatal_recovery
> 
> At this point, we have pci_dev pointers in e_info->dev[] that are
> potentially stale.  Right now nobody uses them, but I think it's
> sloppy that we still have them at all.
> 
> I think a better way to fix this would be to call pci_dev_get() in
> add_error_device() when we save the pointer, and then do the
> corresponding pci_dev_put() in aer_isr_one_error(), after we return
> from aer_process_err_devices().
> 
> This could be done with a little helper function that iterates through
> e_info->dev[i], calls pci_dev_put() for each, and clears
> e_info->error_dev_num (which could then be removed from
> find_source_device()).

I tried the request_threaded_irq() method, hoping that it will hold on 
to the device, unfortunately I am still seeing the use-after-free issue. 
I guess the next step is to implement Bjorn's suggestion. If we fix the 
issue in the add_error_device(), that would require DPC to hold on to 
the device. Would Keith has any concern about it?

Thank you,
Thomas

> 
> I think this would fix the problem you're seeing, so we wouldn't need
> the change in pcie_do_fatal_recovery().
> 
> However, I think we're also slightly exposed in dpc_work(), in basically
> the same (possibly harmless) way.
> 
>    dpc_irq
>      schedule_work(&dpc->work)
>    ...
>    dpc_work
>      pdev = dpc->dev->port
>      pcie_do_fatal_recovery(pdev)
> 
> pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
> holding onto a pointer (which it never uses again).
> 
> The DPC driver should be holding a reference to pdev (through some black
> magic I don't understand), but that would be released when pdev is removed,
> and I don't know what ensures that dpc_work() runs before that release.
> 
> Bjorn
>