Re: [RFC PATCH v1] PCI: Remove pcie_flr() and convert all callers to use pcie_reset_flr()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unchecked VF FLR failure introduces a reliable DMA Use-After-Free (UAF) in the MANA driver during queue teardown.
- [High] Replacing `pcie_flr()` with `pcie_reset_flr()` silently breaks Function Level Reset (FLR) for all SR-IOV Virtual Functions (VFs) across multiple drivers.
- [High] Silent FLR failure in PCI reset quirks causes the kernel to block for 20 seconds during device teardown or initialization.
- [Low] The patch breaks the kernel build because it missed converting a caller of `pcie_flr()` in the `ixgbe` driver.

Pre-existing issues:
- [Critical] An unbounded MMIO read loop causes an infinite loop and hard lockup if the PCIe device is hot-unplugged or encounters a fatal hardware error.
--

commit e494ba565a5af359d72294da06691da5e6507322
Author: Farhan Ali <alifm@linux.ibm.com>
Subject: PCI: Remove pcie_flr() and convert all callers to use pcie_reset_flr()

This patch removes the unconditional pcie_flr() function and converts its
remaining callers to use pcie_reset_flr(). This ensures validation checks,
such as verifying FLR support in the device capabilities, are applied
consistently across the kernel.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
>  int pcie_reset_flr(struct pci_dev *dev, bool probe)
>  {
>  	int ret;
>  
>  	if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
>  		return -ENOTTY;
>  
>  	if (!(dev->devcap & PCI_EXP_DEVCAP_FLR))
>  		return -ENOTTY;

[Severity: High]
According to the PCIe Base Specification, SR-IOV Virtual Functions do not
report FLR capability in their Device Capabilities register, so this check
will fail for VFs. 

Is this a regression that inadvertently breaks FLR for VFs that previously
relied on pcie_flr() bypassing this check? Callers that ignore the return
value will silently skip resetting the hardware.


> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -8319,7 +8319,7 @@ static void ixgbe_check_for_bad_vf(struct ixgbe_adapter *adapter)
>  		if (status_reg != IXGBE_FAILED_READ_CFG_WORD &&
>  		    status_reg & PCI_STATUS_REC_MASTER_ABORT) {
>  			ixgbe_bad_vf_abort(adapter, vf);
> -			pcie_flr(vfdev);
> +			pcie_reset_flr(vfdev, PCI_RESET_DO_RESET);
>  		}

[Severity: Low]
Does this patch introduce a build regression by missing another caller in
ixgbe_main.c? 

Searching the tree might reveal that ixgbe_io_error_detected() still calls
the now-removed pcie_flr(), which would cause a build failure when compiling
with CONFIG_IXGBE.


> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -3306,7 +3306,8 @@ static int mana_dealloc_queues(struct net_device *ndev)
>  			}
>  			if (atomic_read(&txq->pending_sends)) {
>  				err =
> -				    pcie_flr(to_pci_dev(gd->gdma_context->dev));
> +				    pcie_reset_flr(to_pci_dev(gd->gdma_context->dev),
> +						   PCI_RESET_DO_RESET);
>  				if (err) {
>  					netdev_err(ndev, "flr failed %d with %d pkts pending in txq %u\n",
>  						   err,

[Severity: Critical]
If pcie_reset_flr() returns -ENOTTY for VFs, the FLR fails and this loop
breaks early. 

Will the driver then proceed to unmap and free active SKB buffers in the
subsequent loop:

mana_dealloc_queues() {
        ...
        for (i = 0; i < apc->num_queues; i++) {
                txq = &apc->tx_qp[i].txq;
                while ((skb = skb_dequeue(&txq->pending_skbs))) {
                        mana_unmap_skb(skb, apc);
                        dev_kfree_skb_any(skb);
                }
        ...
}

Since the hardware wasn't reset and remains active, can this lead to a DMA
use-after-free regression as the hardware continues to process the freed
memory?


> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
[ ... ]
> @@ -4207,7 +4209,7 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>  	val = val | HINIC_VF_FLR_PROC_BIT;
>  	iowrite32be(val, bar + HINIC_VF_OP);
>  
> -	pcie_flr(pdev);
> +	pcie_reset_flr(pdev, PCI_RESET_DO_RESET);
>  
>  	/*
>  	 * The device must recapture its Bus and Device Numbers after FLR

[Severity: High]
If pcie_reset_flr() returns -ENOTTY without actually issuing a reset to the
hardware, will the firmware ever clear the HINIC_VF_FLR_PROC_BIT? 

Looking at the rest of reset_hinic_vf_dev(), it appears the code will
subsequently spin in a busy-wait loop for the full 20-second timeout
waiting for that bit to clear:

reset_hinic_vf_dev() {
        ...
        do {
                val = ioread32be(bar + HINIC_VF_OP);
                if (!(val & HINIC_VF_FLR_PROC_BIT))
                        goto reset_complete;
                msleep(20);
        } while (time_before(jiffies, timeout));
        ...
}

Could this regression silently block the thread for 20 seconds during
device teardown?


> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c
[ ... ]
> @@ -802,7 +802,7 @@ static int netc_timer_pci_probe(struct pci_dev *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	pcie_flr(pdev);
> +	pcie_reset_flr(pdev, PCI_RESET_DO_RESET);
>  	err = pci_enable_device_mem(pdev);

[Severity: Critical]
This is a pre-existing issue, but while reviewing this driver's PCIe
handling, does the interrupt handler have an unbounded loop on PCIe
disconnect?

In netc_timer_handle_etts_event(), the code continuously reads a
memory-mapped register:

netc_timer_handle_etts_event() {
        ...
        while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index)) {
                ...
        }
        ...
}

If the device is removed or experiences a fatal error, the MMIO read will
return all 1s (0xFFFFFFFF). Because 0xFFFFFFFF masked with the bit is
always true, would this loop spin infinitely and cause a hard lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615192200.3489-1-alifm@linux.ibm.com?part=1