Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI/AER: false Root Port match in is_error_source() skips downstream search
--

commit 4a4627301e6c414f9fd5fc732ea651b5b76f3b16
Author: Lukas Wunner <lukas@wunner.de>

PCI/AER: Check validity of Error Source ID register

This commit replaces the bus-zero heuristic for detecting invalid AER Error
Source IDs with a check against the Root Port's subordinate bus range or an
RCEC's associated buses and devices, and logs when an invalid source ID is
detected.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=222095

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index d492c7ce01e15..9957c36bf215f 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c

[ ... ]

> @@ -1146,6 +1147,24 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static bool source_id_valid(struct pci_dev *root, u16 id)
> +{
> +	/* The Root Port or RCEC can itself be a valid Error Source */
> +	if (pci_dev_id(root) == id)
> +		return true;
> +
> +	/* For RCECs, @id shall be in range of its associated buses/devices */
> +	if (pci_pcie_type(root) == PCI_EXP_TYPE_RC_EC)
> +		return pci_rcec_associated(root, id);
> +
> +	/* For Root Ports, @id shall be in range of its subordinate buses */
> +	if (!root->subordinate)
> +		return false;
> +
> +	return root->subordinate->busn_res.start <= PCI_BUS_NUM(id) &&
> +	       root->subordinate->busn_res.end   >= PCI_BUS_NUM(id);
> +}
> +
>  /**
>   * is_error_source - check whether the device is source of reported error
>   * @dev: pointer to pci_dev to be checked
> @@ -1157,11 +1176,8 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
>  	u32 status, mask;
>  	u16 reg16;
>  
> -	/*
> -	 * When bus ID is equal to 0, it might be a bad ID
> -	 * reported by Root Port.
> -	 */
> -	if ((PCI_BUS_NUM(e_info->id) != 0) &&
> +	/* Some Root Ports report an invalid Error Source ID */
> +	if (e_info->id_valid &&
>  	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
>  		/* Device ID match? */
>  		if (e_info->id == pci_dev_id(dev))

[Severity: High]
Can this cause a false positive match on the Root Port itself and skip
searching the subordinate hierarchy when a buggy Root Port reports 0x0000
(or loses the bus number of a downstream error source)?

Previously, in drivers/pci/pcie/aer.c, when find_source_device() checked a
Root Port on bus 0 via find_device_iter() -> is_error_source(), the check
PCI_BUS_NUM(e_info->id) != 0 was false whenever e_info->id was on bus 0.
As a result, is_error_source() never matched the Root Port on bus 0 by ID
alone, and instead fell through to check whether the Root Port actually had
an unmasked error bit set in PCI_ERR_COR_STATUS or PCI_ERR_UNCOR_STATUS.

With this change, aer_isr_one_error_type() sets info->id_valid via
source_id_valid(), which returns true whenever pci_dev_id(root) == id. On
platforms where the Root Port itself is at 00:00.0 (such as many SoC PCIe
controllers or VMD domains), pci_dev_id(root) is 0x0000. The same condition
also occurs if a Root Port at 00:xx.y loses the bus number of a downstream
endpoint at bus:xx.y (the failure mode described in commit 28eb27cf0839), or
 similarly for an RCEC on bus 0 with slot 0 set in rcec->rcec_ea->bitmap.

When a downstream device signals an error and PCI_ERR_ROOT_ERR_SRC reads as
pci_dev_id(root), source_id_valid() sets info->id_valid = 1. Then in
find_source_device():

  find_source_device()
    find_device_iter(root, e_info)
      is_error_source(root, e_info)
        if (e_info->id_valid &&
            !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
                if (e_info->id == pci_dev_id(dev))
                        return true;

is_error_source() immediately returns true for the Root Port without
checking its AER status registers, and when e_info->multi_error_valid is 0,
find_source_device() returns early without walking parent->subordinate:

  drivers/pci/pcie/aer.c:find_source_device() {
      ...
      /* Is Root Port an agent that sends error message? */
      result = find_device_iter(dev, e_info);
      if (result)
          return true;

      if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
          pcie_walk_rcec(parent, find_device_iter, e_info);
      else if (parent->subordinate)
          pci_walk_bus(parent->subordinate, find_device_iter, e_info);
  }

Subsequently, aer_process_err_devices() calls aer_get_device_error_info()
on the Root Port, sees !(info->status & ~info->mask), and returns 0, leaving
the downstream device's error unlogged, uncleared, and unrecovered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793697480.git.lukas@wunner.de?part=2