Re: [PATCH v1 5/5] iommu: Pass in reset result to pci_dev_reset_iommu_done()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu: nested FLR failure lost in pci_dev_reset_iommu_done() and quirks
--

commit bcd9c215d1b85e6e5dddae2f6e5dddae2f6e342b6ed1a3b197
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu: Pass in reset result to pci_dev_reset_iommu_done()

This commit passes the return value of PCI reset functions into
pci_dev_reset_iommu_done() so that on a failed reset the device remains
attached to the blocking domain with group->recovery_cnt kept elevated.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0d40eb4f15cc1..1e3a243a6f1ef 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
[ ... ]
> @@ -4138,20 +4152,28 @@ static bool group_device_dma_alias_is_blocked(struct iommu_group *group,
>  /**
>   * pci_dev_reset_iommu_done() - Restore IOMMU after a PCI device reset is done
>   * @pdev: PCI device that has finished a reset routine
> + * @reset_result: Return code from the reset routine
>   *
>   * After a PCIe device finishes a reset routine, it wants to restore its IOMMU
>   * activity, including new translation and cache invalidation, by re-attaching
>   * all RID/PASID of the device back to the domains retained in the core-level
>   * structure.
>   *
> - * Caller must pair it with a successful pci_dev_reset_iommu_prepare().
> + * This is a pairing function for pci_dev_reset_iommu_prepare(). Caller passes
> + * the reset return value to @reset_result. On a failed reset, the device will
> + * remain blocked as a quarantine measure, with group->recovery_cnt intact, to
> + * protect system memory until a subsequent successful reset.
> + *
> + * Callers must skip pci_dev_reset_iommu_prepare/done() entirely when no reset
> + * is attempted (e.g. probe mode).
>   *
>   * Note that, although unlikely, there is a risk that re-attaching domains might
>   * fail due to some unexpected happening like OOM.
>   */
> -void pci_dev_reset_iommu_done(struct pci_dev *pdev)
> +void pci_dev_reset_iommu_done(struct pci_dev *pdev, int reset_result)
>  {
>  	struct iommu_group *group = pdev->dev.iommu_group;
> +	enum blocked_reason old_blocked;
>  	struct group_device *gdev;
>  	unsigned long pasid;
>  	void *entry;
> @@ -4174,6 +4196,37 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev)
>  	if (WARN_ON(!group->blocking_domain))
>  		return;
>  
> +	/*
> +	 * A reset failure implies that the device might be unreliable. E.g. its
> +	 * device cache might retain stale entries, which might result in memory
> +	 * corruption. Thus, do not unblock the device until a successful reset.
> +	 */
> +	if (reset_result) {
> +		/*
> +		 * FIXME: the int-return values from the PCI reset functions are
> +		 * not consistent: some reset functions use -ENOTTY to indicate
> +		 * "no reset was attempted" (in which case IOMMU should revert a
> +		 * prepare), while others use -ENOTTY to indicate "reset failed;
> +		 * try the next reset method" (in which case IOMMU should keep
> +		 * the device blocked). Without fixing the PCI return result, we
> +		 * cannot tell the difference between the two cases. Warn it.
> +		 */
> +		if (reset_result == -ENOTTY)
> +			dev_warn_ratelimited(
> +				&pdev->dev,
> +				"Reset may have been skipped. Keep it blocked conservatively\n");
> +		else
> +			dev_err_ratelimited(
> +				&pdev->dev,
> +				"Reset failed. Keep it blocked to protect memory\n");
> +		if (gdev->blocked == BLOCKED_RESETTING)
> +			gdev->blocked = BLOCKED_RESET_FAILED;
> +		return;
> +	}
[ ... ]
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 3d3e95cb9f7dc..92e81460c2d8f 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4416,7 +4416,7 @@ static int __pci_dev_specific_reset(struct pci_dev *dev, bool probe,
>  
>  	ret = i->reset(dev, probe);
>  	if (!probe)
> -		pci_dev_reset_iommu_done(dev);
> +		pci_dev_reset_iommu_done(dev, ret);
>  	return ret;
>  }

[Severity: High]
Can a nested FLR failure inside a device-specific reset quirk be lost,
allowing the device to be unblocked even when the underlying FLR failed?

When __pci_dev_specific_reset() runs, it calls pci_dev_reset_iommu_prepare()
which increments gdev->reset_depth from 0 to 1, attaches
group->blocking_domain, and sets gdev->blocked = BLOCKED_RESETTING.

Several callbacks in pci_dev_reset_methods[] then invoke pcie_flr() or
pcie_reset_flr() without propagating its return value, such as
nvme_disable_and_flr(), delay_250ms_after_flr(),
reset_intel_82599_sfp_virtfn(), reset_chelsio_generic_dev(), and
reset_hinic_vf_dev():

drivers/pci/quirks.c:nvme_disable_and_flr() {
	...
	pci_iounmap(dev, bar);

	pcie_flr(dev);

	return 0;
}

drivers/pci/quirks.c:delay_250ms_after_flr() {
	if (probe)
		return pcie_reset_flr(dev, PCI_RESET_PROBE);

	pcie_reset_flr(dev, PCI_RESET_DO_RESET);

	msleep(250);

	return 0;
}

Inside pcie_flr(), pci_dev_reset_iommu_prepare() increments
gdev->reset_depth from 1 to 2. If the FLR fails (for example, pci_dev_wait()
times out or sees a disconnected device and returns -ENOTTY), pcie_flr()
calls pci_dev_reset_iommu_done(dev, ret) with ret = -ENOTTY.

However, pci_dev_reset_iommu_done() checks gdev->reset_depth before
inspecting reset_result:

drivers/iommu/iommu.c:pci_dev_reset_iommu_done() {
	...
	if (WARN_ON(gdev->reset_depth == 0))
		return;
	if (--gdev->reset_depth)
		return;

	if (WARN_ON(!group->blocking_domain))
		return;

	if (reset_result) {
	...
}

Because --gdev->reset_depth is still 1 on the inner call,
pci_dev_reset_iommu_done() returns early and discards the inner failure.
(Similarly, if pcie_flr() fails at !pci_dev_config_accessible(), it returns
-ENOTTY before calling pci_dev_reset_iommu_prepare(), and reset_hinic_vf_dev()
and reset_ivb_igd() also return 0 on post-reset polling timeouts.)

The quirk callback then returns 0 to __pci_dev_specific_reset(), which calls
pci_dev_reset_iommu_done(dev, 0). At that outer call, --gdev->reset_depth
reaches 0 with reset_result == 0, so pci_dev_reset_iommu_done() unblocks the
device (even clearing a prior BLOCKED_RESET_FAILED quarantine), re-attaches
group->domain and all PASID domains, and decrements group->recovery_cnt.

Should the FLR quirk callbacks propagate the return value of pcie_flr() /
pcie_reset_flr(), and should pci_dev_reset_iommu_done() also record a
non-zero reset_result before returning when gdev->reset_depth > 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793729707.git.nicolinc@nvidia.com?part=5