lkml.org 
[lkml]   [2013]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [3.11.4] Thunderbolt/PCI unplug oops in pci_pme_list_scan
    On Thu, Oct 24, 2013 at 11:13 PM, Yinghai Lu <yinghai@kernel.org> wrote:
    > On Thu, Oct 24, 2013 at 8:33 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
    >>
    >>>>> Bisection points to 928bea964827d7824b548c1f8e06eccbbc4d0d7d .
    >>>>
    >>>> This is "PCI: Delay enabling bridges until they're needed" by Yinghai.
    >>>
    >>> that double disabling should be addressed by:
    >>>
    >>> https://lkml.org/lkml/2013/4/25/608
    >>>
    >>> [PATCH] PCI: Remove duplicate pci_disable_device for pcie port
    >>
    >> I'll look at that patch again. I had some questions about it the
    >> first time, but perhaps it makes more sense after 928bea9648 has been
    >> applied.
    >>
    >> Andreas originally reported a GPF oops in pci_pme_list_scan(). I
    >> posted a refcounting patch, which made the problem go away, but I
    >> can't explain why, and I don't want to apply it without understanding
    >> that. Decoding his oops shows this:
    >>
    >> 24: 0f 1f 00 nopl (%rax)
    >> 27: 48 8b 50 10 mov 0x10(%rax),%rdx
    >> 2b:* 48 8b 52 38 mov 0x38(%rdx),%rdx <-- trapping instruction
    >> 2f: 48 85 d2 test %rdx,%rdx
    >>
    >> %rax is the pci_dev pointer, so 0x10(%rax) is the dev->bus pointer,
    >> which we put in %rdx. The oops says %rdx = 6b6b6b6b6b6b6b6b, which is
    >> POISON_FREE, so I think we loaded dev->bus out of a struct pci_dev
    >> that has already been freed.
    >>
    >> pci_pme_list_scan() holds pci_pme_list_mutex while it traverses
    >> pci_pme_list, and the pci_stop_and_remove_bus_device() path removes
    >> the pci_dev by calling pci_pme_active(), which also holds
    >> pci_pme_list_mutex, so I don't understand how pci_pme_list_scan() can
    >> see a pci_dev that has already been freed.
    >>
    >> If I understand Andreas correctly, 928bea9648 also fixes the crash,
    >> even without my refcounting change. Can you explain why?
    >
    > 928bea will make the dev->enable_cnt increase wrongly, as we have
    > pci_enable_device for child
    > pci_enable_bridge for parent
    > pci_enable_bridge for grandparent
    > pci_enable_device for grandparent
    > pci_enable_device for parent
    > pci_enable_brdige for grandparent
    > pci_enable_device for grandparent.
    > ...
    >
    > in that case grandprent will be enabled two times, and will enable_cnt will have
    > extra increase.
    >
    > so later pci_disable_device will not really call do_pci_disable_device
    > do the really work, as enable_cnt still big.
    >
    > solution could be:
    > let pci_enable_bridge call __pci_enable_device.
    > and __pci_enable_device will not call pci_enable_bridge.

    Sorry, I didn't understand this. Is this supposed to be an
    explanation of how 928bea fixes the oops that Andreas saw? If so, can
    you be a little more explicit about when the pci_dev got freed and
    when pci_pme_list_scan() walked the list and accessed the freed area?

    Bjorn


    \
     
     \ /
      Last update: 2013-10-26 01:41    [W:2.926 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site