Re: [PATCH v2 2/5] iommu/amd: Fix DTE clearing and rename iommu_ignore_device()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The condition `if (dev_iommu_priv_get(dev))` in `iommu_disable_device_dma()` always evaluates to false on the probe error path, making the alias DTE cleanup dead code.
- [High] Lockless execution of atomic DTE write helpers risks silent update failures and IRQ remapping corruption.

Pre-existing issues:
- [High] Missing hardware IOMMU cache flush after disabling device DMA.
--

--- Patch [2]: [PATCH v2 2/5] iommu/amd: Fix DTE clearing and rename iommu_ignore_device() ---
commit fe0ff8dabc53d6febbad3289c8dca1c1bf0c2df2
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Fix DTE clearing and rename iommu_ignore_device()

This commit replaces the manual memset of the primary Device Table Entry
with a dedicated helper to avoid torn writes. It explicitly clears the
DTE lower and upper bits, and renames the function to
iommu_disable_device_dma.

Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index ef21fbd9c9a89..91d551f29ed12 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c

[ ... ]

> @@ -2243,6 +2227,41 @@ static void dev_update_dte(struct iommu_dev_data *dev_data, bool set)
>  		clear_dte_entry(iommu, dev_data);
>  }
>  
> +/*
> + * Invalidate a DTE by clearing the Valid bit first.
> + * Note: Not to be used on a fully probed device with
> + * live dev_data.
> + */
> +static void amd_iommu_disable_dte(struct dev_table_entry *ptr)
> +{
> +	struct dev_table_entry new = {};
> +
> +	write_dte_lower128(ptr, &new);
> +	write_dte_upper128(ptr, &new);
> +}

[Severity: High]
Does this sequence need to be protected by a lock?

Looking at write_dte_upper128(), it performs a read-modify-write of the
interrupt mask:

write_dte_upper128(ptr, new)
    old.data128[1] = ptr->data128[1];
    new->data[2] &= ~DTE_DATA2_INTR_MASK;
    new->data[2] |= old.data[2] & DTE_DATA2_INTR_MASK;
    amd_iommu_atomic128_set(&ptr->data128[1], new->data128[1]);

The comments in write_dte_upper128() mention this requires being inside
a spin_lock(&dev_data->dte_lock) context. If this is called locklessly,
could a concurrent modification (such as clone_aliases() from another
device probing) cause the underlying cmpxchg16b to silently fail or
corrupt the IRQ remapping masks?

> +
> +static void iommu_disable_device_dma(struct amd_iommu *iommu, struct device *dev)
> +{
> +	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	int devid, sbdf;
> +
> +	sbdf = get_device_sbdf_id(dev);
> +	if (sbdf < 0)
> +		return;
> +
> +	devid = PCI_SBDF_TO_DEVID(sbdf);
> +
> +	/* Clear the primary DTE */
> +	amd_iommu_disable_dte(&dev_table[devid]);

[Severity: High]
This is a pre-existing issue, but does the IOMMU hardware cache need to be
flushed after clearing the primary DTE in memory here?

Since the DTE is cleared in memory without a subsequent hardware flush
command (like iommu_flush_dte_sync() or iommu_completion_wait()) before
returning, could the IOMMU hardware continue using the old cached DTE and
retain DMA privileges on failure paths?

> +
> +	/* Clone the cleared DTE to all aliases before wiping the rlookup */
> +	if (dev_iommu_priv_get(dev))
> +		clone_aliases(iommu, dev);

[Severity: High]
Will this condition ever be true when called from the probe error path?

Looking at amd_iommu_probe_device(), it calls iommu_disable_device_dma()
only when iommu_init_device() fails:

amd_iommu_probe_device()
    dev_data = iommu_init_device(iommu, dev, devid);
    if (IS_ERR(dev_data)) {
        ...
        goto err_deinit;

err_deinit:
    iommu_disable_device_dma(iommu, dev);

Since dev_iommu_priv_set() is called inside iommu_init_device(), if the
initialization fails, dev_iommu_priv_get(dev) would return NULL. 
Doesn't this mean clone_aliases() is completely skipped during cleanup on
this error path, leaving DMA aliases active?

> +
> +	pci_seg->rlookup_table[devid] = NULL;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=2