Page MenuHomeFreeBSD

pci: stop the capability walk spinning on an unreachable device
Needs ReviewPublic

Authored by ken on Mon, Oct 5, 2:49 PM.

Details

Reviewers
imp
jhb
Summary

A device that has been surprise-removed answers every configuration
read with all-ones. pci_find_cap_method() did not test for that, and
an all-ones PCIR_STATUS has PCIM_STATUS_CAPPRESENT set, so the
CAP_LIST check passed and the traversal followed a capability list in
which every next-pointer read returned 0xff. Since 0xff is never 0,
the walk ran to its full (PCIE_REGMAX - 0x40) / 2 bound - roughly 4000
configuration reads, each one paying a PCIe completion timeout.

The reads are issued from a sysctl handler running under Giant, so
this does not merely waste time in the caller. On one of our systems,
after an NVMe boot drive took a write failure and detached, a single
read of the dev.nvme.N.%iommu sysctl on the dead PCI function - which
reaches pci_find_cap_method() by way of iommu_get_requester() walking
up the bridge hierarchy - spun a core for over 20 minutes at roughly
330ms per configuration read, and left ps, sockstat and ipmitool
spinning on the same lock. The thread never returns to userland
during the walk, so the process cannot be signalled - kill -9 leaves
it running. sysctl -a reaches that node, so any periodic monitoring
job that runs sysctl -a wedges the machine on a schedule:

PID    TID    COMM     KSTACK
28031  102638 sysctl   pci_find_cap_method+0x19c
                       iommu_get_requester+0x192
                       device_sysctl_handler+0x200
                       sysctl_root_handler_locked+0x91
                       sysctl_root+0x268 userland_sysctl+0x188
                       sys___sysctl+0x65 amd64_syscall+0x117
                       fast_syscall_common+0xf8

Check for an all-ones PCIR_STATUS before walking anything: the low
bits of the status register are reserved and never set on a device
that is really there, so an all-ones status cannot be genuine. Also
stop the traversal on an all-ones next pointer; 0xff is not a legal
capability offset in any case (not DWORD-aligned, and a capability
needs two bytes, which would run past the 256-byte configuration
space). A device that returns 0 instead of all-ones was already
handled by the existing CAPPRESENT check.

pci_find_extcap_method() already rejects an all-ones read for the same
reason, which is why the extended capability walk is immune and only
the legacy walk was affected. This brings the two into line rather
than inventing a convention.

The patched kernel has been built and booted on a 14.5-based
production system, where normal capability lookups across the whole
PCI tree are unaffected - pci_find_cap_method() runs during every PCI
device attach - and both guards were confirmed present in the
compiled object. The failing path itself has not been re-run on
hardware with a surprise-removed device yet; the expected behavior
with the fix is that sysctl -a completes normally and reading %iommu
on the dead function returns immediately with no output, which is the
correct answer for a device that is gone.

Sponsored by: Spectra Logic

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 77718
Build 74601: arc lint + arc unit

Event Timeline

ken requested review of this revision.Mon, Oct 5, 2:49 PM
ken created this revision.