[v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

[PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by David Matlack 5 days, 15 hours ago

Ensure that PCI devices with ATS disabled via quirk have it disabled
before IOMMU drivers are notified about the device rather than after.
Fix this by converting the existing quirks from final to early fixups
and changing the quirk logic to set a new no_ats bit in struct pci_dev
that prevents pci_dev.ats_cap from ever gettting set.

This change ensures that pci_ats_supported() takes quirks into account
during iommu_ops.probe_device(), when IOMMU drivers are first notified
about devices. It also ensures that pci_ats_supported() returns the same
value when the device is released in iommu_ops.release_device().

Notably, the Intel IOMMU driver uses pci_ats_supported() in
probe/release to determine whether to add/remove a device from a data
structure, which easily leads to a use-after-free without this fix.

This change also makes disabling ATS via quirk behave the same way as
the pci=noats command line option, in that pci_ats_init() bails
immediately and never intializes pci_dev.ats_cap.

Fixes: a18615b1cfc0 ("PCI: Disable ATS for specific Intel IPU E2000 devices")
Closes: https://lore.kernel.org/linux-iommu/aYUQ_HkDJU9kjsUl@google.com/
Signed-off-by: David Matlack <dmatlack@google.com>
---
v2:
 - Update the commit message with reasons why this is being fixed in the
   PCI core, rather than applying a point fix to the Intel IOMMU driver
   (Andy)
 - Condense the pci_ats_disabled() and dev->no_ats checks into a single
   line in pci_ats_init()
 - Reorder the no_ats bitfield to be after ats_stu since there is likely
   u8-sized gap there for alignment purposes

v1: https://lore.kernel.org/linux-pci/20260223184017.688212-1-dmatlack@google.com/

Cc: Raghavendra Rao Ananta <rananta@google.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

 drivers/pci/ats.c    |  2 +-
 drivers/pci/quirks.c | 50 ++++++++++++++++++++++----------------------
 include/linux/pci.h  |  1 +
 3 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index ec6c8dbdc5e9..ceb6f5d3cb10 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -21,7 +21,7 @@ void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
 
-	if (pci_ats_disabled())
+	if (pci_ats_disabled() || dev->no_ats)
 		return;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 48946cca4be7..2c7e11830e45 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5653,7 +5653,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0422, quirk_no_ext_tags);
 static void quirk_no_ats(struct pci_dev *pdev)
 {
 	pci_info(pdev, "disabling ATS\n");
-	pdev->ats_cap = 0;
+	pdev->no_ats = 1;
 }
 
 /*
@@ -5676,25 +5676,25 @@ static void quirk_amd_harvest_no_ats(struct pci_dev *pdev)
 }
 
 /* AMD Stoney platform GPU */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x98e4, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x98e4, quirk_amd_harvest_no_ats);
 /* AMD Iceland dGPU */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6900, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x6900, quirk_amd_harvest_no_ats);
 /* AMD Navi10 dGPU */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7310, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7312, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7318, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7319, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731a, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731b, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731e, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731f, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7310, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7312, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7318, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7319, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731a, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731b, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731e, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731f, quirk_amd_harvest_no_ats);
 /* AMD Navi14 dGPU */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7340, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7341, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7341, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
 /* AMD Raven platform iGPU */
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
 
 /*
  * Intel IPU E2000 revisions before C0 implement incorrect endianness
@@ -5705,15 +5705,15 @@ static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
 	if (pdev->revision < 0x20)
 		quirk_no_ats(pdev);
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
 #endif /* CONFIG_PCI_ATS */
 
 /* Freescale PCIe doesn't support MSI in RC mode */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1c270f1d5123..850100f209e3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -539,6 +539,7 @@ struct pci_dev {
 	};
 	u16		ats_cap;	/* ATS Capability offset */
 	u8		ats_stu;	/* ATS Smallest Translation Unit */
+	unsigned int	no_ats:1;	/* ATS disabled via quirk */
 #endif
 #ifdef CONFIG_PCI_PRI
 	u16		pri_cap;	/* PRI Capability offset */

base-commit: 7df48e36313029e4c0907b2023905dd7213fd678
-- 
2.53.0.1018.g2bb0e51243-goog

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by David Matlack 1 day, 20 hours ago

On Fri, Mar 27, 2026 at 2:17 PM David Matlack <dmatlack@google.com> wrote:

>  /* AMD Raven platform iGPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);

Sashiko found a bug here that would leave ATS enabled on this device:

  https://sashiko.dev/#/patchset/20260327211649.3816010-1-dmatlack%40google.com?patch=12646

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by Bjorn Helgaas 1 day, 18 hours ago

On Tue, Mar 31, 2026 at 09:01:53AM -0700, David Matlack wrote:
> On Fri, Mar 27, 2026 at 2:17 PM David Matlack <dmatlack@google.com> wrote:
> 
> >  /* AMD Raven platform iGPU */
> > -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> 
> Sashiko found a bug here that would leave ATS enabled on this device:
> 
>   https://sashiko.dev/#/patchset/20260327211649.3816010-1-dmatlack%40google.com?patch=12646

Do you want to update the patch to use DECLARE_PCI_FIXUP_HEADER
instead of DECLARE_PCI_FIXUP_EARLY, as Sashiko suggested?

I don't know the IOMMU init ordering with respect to PCI enumeration,
but header fixups are run in pci_device_add() before it calls
device_add().

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by David Matlack 1 day, 17 hours ago

On Tue, Mar 31, 2026 at 11:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Mar 31, 2026 at 09:01:53AM -0700, David Matlack wrote:
> > On Fri, Mar 27, 2026 at 2:17 PM David Matlack <dmatlack@google.com> wrote:
> >
> > >  /* AMD Raven platform iGPU */
> > > -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> > > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> >
> > Sashiko found a bug here that would leave ATS enabled on this device:
> >
> >   https://sashiko.dev/#/patchset/20260327211649.3816010-1-dmatlack%40google.com?patch=12646
>
> Do you want to update the patch to use DECLARE_PCI_FIXUP_HEADER
> instead of DECLARE_PCI_FIXUP_EARLY, as Sashiko suggested?
>
> I don't know the IOMMU init ordering with respect to PCI enumeration,
> but header fixups are run in pci_device_add() before it calls
> device_add().

Yes I would like to work on another version. I have 3 things I want to
investigate/fix:

 * Fix the bug Sashiko found with early vs. header fixups.
 * Investigate whether no_ats be a u8 instead of unsigned int to avoid
increasing sizeof(struct pci_dev).
 * Investigate Boalu's finding. Maybe this issue is specific to VF
creation, and changing the order of calls there would be a better
solution.

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by Bjorn Helgaas 1 day, 17 hours ago

On Tue, Mar 31, 2026 at 11:46:22AM -0700, David Matlack wrote:
> On Tue, Mar 31, 2026 at 11:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Mar 31, 2026 at 09:01:53AM -0700, David Matlack wrote:
> > > On Fri, Mar 27, 2026 at 2:17 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > >  /* AMD Raven platform iGPU */
> > > > -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> > > > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> > >
> > > Sashiko found a bug here that would leave ATS enabled on this device:
> > >
> > >   https://sashiko.dev/#/patchset/20260327211649.3816010-1-dmatlack%40google.com?patch=12646
> >
> > Do you want to update the patch to use DECLARE_PCI_FIXUP_HEADER
> > instead of DECLARE_PCI_FIXUP_EARLY, as Sashiko suggested?
> >
> > I don't know the IOMMU init ordering with respect to PCI enumeration,
> > but header fixups are run in pci_device_add() before it calls
> > device_add().
> 
> Yes I would like to work on another version. I have 3 things I want to
> investigate/fix:
> 
>  * Fix the bug Sashiko found with early vs. header fixups.
>  * Investigate whether no_ats be a u8 instead of unsigned int to avoid
> increasing sizeof(struct pci_dev).
>  * Investigate Boalu's finding. Maybe this issue is specific to VF
> creation, and changing the order of calls there would be a better
> solution.

OK, I'll drop this one for now and watch for an update.

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by Andy Shevchenko 1 day, 17 hours ago

On Tue, Mar 31, 2026 at 11:46:22AM -0700, David Matlack wrote:
> On Tue, Mar 31, 2026 at 11:38 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Mar 31, 2026 at 09:01:53AM -0700, David Matlack wrote:
> > > On Fri, Mar 27, 2026 at 2:17 PM David Matlack <dmatlack@google.com> wrote:

...

> Yes I would like to work on another version. I have 3 things I want to
> investigate/fix:
> 
>  * Fix the bug Sashiko found with early vs. header fixups.

>  * Investigate whether no_ats be a u8 instead of unsigned int to avoid
> increasing sizeof(struct pci_dev).

Use `pahole`, it will check the layout and suggests correctly what's going on
there. It's not a fuzzy AI :-)

>  * Investigate Boalu's finding. Maybe this issue is specific to VF
> creation, and changing the order of calls there would be a better
> solution.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by Baolu Lu 2 days, 9 hours ago

On 3/28/26 05:16, David Matlack wrote:
> Ensure that PCI devices with ATS disabled via quirk have it disabled
> before IOMMU drivers are notified about the device rather than after.
> Fix this by converting the existing quirks from final to early fixups
> and changing the quirk logic to set a new no_ats bit in struct pci_dev
> that prevents pci_dev.ats_cap from ever gettting set.
> 
> This change ensures that pci_ats_supported() takes quirks into account
> during iommu_ops.probe_device(), when IOMMU drivers are first notified
> about devices. It also ensures that pci_ats_supported() returns the same
> value when the device is released in iommu_ops.release_device().
> 
> Notably, the Intel IOMMU driver uses pci_ats_supported() in
> probe/release to determine whether to add/remove a device from a data
> structure, which easily leads to a use-after-free without this fix.

Can you please shed more light on the above issue? In my investigation,
iommu_ops.probe_device() is always called after the no_ats quirk,
regardless of whether this patch is applied.

The diff of the changes I made for testing is as follows:

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 442271a1b92a..c024964ac53b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3271,6 +3271,8 @@ static struct iommu_device 
*intel_iommu_probe_device(struct device *dev)
                                 info->pfsid = pci_dev_id(pci_physfn(pdev));
                         info->ats_qdep = pci_ats_queue_depth(pdev);
                 }
+               pci_info(pdev, "ATS %s\n", info->ats_supported ?
+                        "supported" : "not supported");
                 if (sm_supported(iommu)) {
                         if (pasid_supported(iommu)) {
                                 int features = pci_pasid_features(pdev);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 48946cca4be7..c63616d108b7 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5714,6 +5714,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 
0x1457, quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, 
quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, 
quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, 
quirk_intel_e2000_no_ats);
+
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0b25, quirk_no_ats);
  #endif /* CONFIG_PCI_ATS */

  /* Freescale PCIe doesn't support MSI in RC mode */


The related kernel messages are shown below:

# dmesg | grep "0000:00:01.0"
[   15.834944] pci 0000:00:01.0: [8086:0b25] type 00 class 0x088000 PCIe 
Root Complex Integrated Endpoint
[   15.836382] pci 0000:00:01.0: BAR 0 [mem 
0x1e0fff980000-0x1e0fff99ffff 64bit pref]
[   15.836655] pci 0000:00:01.0: BAR 2 [mem 
0x1e0fff900000-0x1e0fff93ffff 64bit pref]
[   15.837904] pci 0000:00:01.0: calling 
quirk_igfx_skip_te_disable+0x0/0xe0 @ 1
[   15.838614] pci 0000:00:01.0: quirk_igfx_skip_te_disable+0x0/0xe0 
took 0 usecs
[   21.205177] pci 0000:00:01.0: calling  quirk_no_ats+0x0/0x40 @ 1
[   21.206767] pci 0000:00:01.0: disabling ATS
[   21.207916] pci 0000:00:01.0: quirk_no_ats+0x0/0x40 took 1122 usecs
[   21.305357] pci 0000:00:01.0: DMAR: ATS not supported
[   21.306925] pci 0000:00:01.0: Adding to iommu group 4
[   42.564912] idxd 0000:00:01.0: Intel(R) Accelerator Device (v200)
[   42.568653] probe of 0000:00:01.0 returned 0 after 87413 usecs


Anything I missed?

Thanks,
baolu

> 
> This change also makes disabling ATS via quirk behave the same way as
> the pci=noats command line option, in that pci_ats_init() bails
> immediately and never intializes pci_dev.ats_cap.
> 
> Fixes: a18615b1cfc0 ("PCI: Disable ATS for specific Intel IPU E2000 devices")
> Closes:https://lore.kernel.org/linux-iommu/aYUQ_HkDJU9kjsUl@google.com/
> Signed-off-by: David Matlack<dmatlack@google.com>
> ---
> v2:
>   - Update the commit message with reasons why this is being fixed in the
>     PCI core, rather than applying a point fix to the Intel IOMMU driver
>     (Andy)
>   - Condense the pci_ats_disabled() and dev->no_ats checks into a single
>     line in pci_ats_init()
>   - Reorder the no_ats bitfield to be after ats_stu since there is likely
>     u8-sized gap there for alignment purposes
> 
> v1:https://lore.kernel.org/linux-pci/20260223184017.688212-1- 
> dmatlack@google.com/
> 
> Cc: Raghavendra Rao Ananta<rananta@google.com>
> Cc: David Woodhouse<dwmw2@infradead.org>
> Cc: Lu Baolu<baolu.lu@linux.intel.com>
> Cc: Andy Shevchenko<andriy.shevchenko@linux.intel.com>
> 
>   drivers/pci/ats.c    |  2 +-
>   drivers/pci/quirks.c | 50 ++++++++++++++++++++++----------------------
>   include/linux/pci.h  |  1 +
>   3 files changed, 27 insertions(+), 26 deletions(-)

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by David Matlack 1 day, 21 hours ago

On Mon, Mar 30, 2026 at 8:32 PM Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 3/28/26 05:16, David Matlack wrote:
> > Ensure that PCI devices with ATS disabled via quirk have it disabled
> > before IOMMU drivers are notified about the device rather than after.
> > Fix this by converting the existing quirks from final to early fixups
> > and changing the quirk logic to set a new no_ats bit in struct pci_dev
> > that prevents pci_dev.ats_cap from ever gettting set.
> >
> > This change ensures that pci_ats_supported() takes quirks into account
> > during iommu_ops.probe_device(), when IOMMU drivers are first notified
> > about devices. It also ensures that pci_ats_supported() returns the same
> > value when the device is released in iommu_ops.release_device().
> >
> > Notably, the Intel IOMMU driver uses pci_ats_supported() in
> > probe/release to determine whether to add/remove a device from a data
> > structure, which easily leads to a use-after-free without this fix.
>
> Can you please shed more light on the above issue? In my investigation,
> iommu_ops.probe_device() is always called after the no_ats quirk,
> regardless of whether this patch is applied.
>
> The diff of the changes I made for testing is as follows:
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 442271a1b92a..c024964ac53b 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3271,6 +3271,8 @@ static struct iommu_device
> *intel_iommu_probe_device(struct device *dev)
>                                  info->pfsid = pci_dev_id(pci_physfn(pdev));
>                          info->ats_qdep = pci_ats_queue_depth(pdev);
>                  }
> +               pci_info(pdev, "ATS %s\n", info->ats_supported ?
> +                        "supported" : "not supported");
>                  if (sm_supported(iommu)) {
>                          if (pasid_supported(iommu)) {
>                                  int features = pci_pasid_features(pdev);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 48946cca4be7..c63616d108b7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5714,6 +5714,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> 0x1457, quirk_intel_e2000_no_ats);
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459,
> quirk_intel_e2000_no_ats);
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a,
> quirk_intel_e2000_no_ats);
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c,
> quirk_intel_e2000_no_ats);
> +
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0b25, quirk_no_ats);
>   #endif /* CONFIG_PCI_ATS */
>
>   /* Freescale PCIe doesn't support MSI in RC mode */
>
>
> The related kernel messages are shown below:
>
> # dmesg | grep "0000:00:01.0"
> [   15.834944] pci 0000:00:01.0: [8086:0b25] type 00 class 0x088000 PCIe
> Root Complex Integrated Endpoint
> [   15.836382] pci 0000:00:01.0: BAR 0 [mem
> 0x1e0fff980000-0x1e0fff99ffff 64bit pref]
> [   15.836655] pci 0000:00:01.0: BAR 2 [mem
> 0x1e0fff900000-0x1e0fff93ffff 64bit pref]
> [   15.837904] pci 0000:00:01.0: calling
> quirk_igfx_skip_te_disable+0x0/0xe0 @ 1
> [   15.838614] pci 0000:00:01.0: quirk_igfx_skip_te_disable+0x0/0xe0
> took 0 usecs
> [   21.205177] pci 0000:00:01.0: calling  quirk_no_ats+0x0/0x40 @ 1
> [   21.206767] pci 0000:00:01.0: disabling ATS
> [   21.207916] pci 0000:00:01.0: quirk_no_ats+0x0/0x40 took 1122 usecs
> [   21.305357] pci 0000:00:01.0: DMAR: ATS not supported
> [   21.306925] pci 0000:00:01.0: Adding to iommu group 4
> [   42.564912] idxd 0000:00:01.0: Intel(R) Accelerator Device (v200)
> [   42.568653] probe of 0000:00:01.0 returned 0 after 87413 usecs
>
>
> Anything I missed?

The Closes link above has the details of my investigation:

  https://lore.kernel.org/linux-iommu/aYUQ_HkDJU9kjsUl@google.com/

Can you check if you see the same ordering for VFs? That was the
scenario where I saw the IOMMU drivers notified before final fixups
were applied.

Re: [PATCH v2] PCI: Disable ATS via quirk before notifying IOMMU drivers

Posted by Bjorn Helgaas 2 days, 14 hours ago

On Fri, Mar 27, 2026 at 09:16:40PM +0000, David Matlack wrote:
> Ensure that PCI devices with ATS disabled via quirk have it disabled
> before IOMMU drivers are notified about the device rather than after.
> Fix this by converting the existing quirks from final to early fixups
> and changing the quirk logic to set a new no_ats bit in struct pci_dev
> that prevents pci_dev.ats_cap from ever gettting set.
> 
> This change ensures that pci_ats_supported() takes quirks into account
> during iommu_ops.probe_device(), when IOMMU drivers are first notified
> about devices. It also ensures that pci_ats_supported() returns the same
> value when the device is released in iommu_ops.release_device().
> 
> Notably, the Intel IOMMU driver uses pci_ats_supported() in
> probe/release to determine whether to add/remove a device from a data
> structure, which easily leads to a use-after-free without this fix.
> 
> This change also makes disabling ATS via quirk behave the same way as
> the pci=noats command line option, in that pci_ats_init() bails
> immediately and never intializes pci_dev.ats_cap.
> 
> Fixes: a18615b1cfc0 ("PCI: Disable ATS for specific Intel IPU E2000 devices")
> Closes: https://lore.kernel.org/linux-iommu/aYUQ_HkDJU9kjsUl@google.com/
> Signed-off-by: David Matlack <dmatlack@google.com>

Applied to pci/enumeration for v7.1, thank you!

> ---
> v2:
>  - Update the commit message with reasons why this is being fixed in the
>    PCI core, rather than applying a point fix to the Intel IOMMU driver
>    (Andy)
>  - Condense the pci_ats_disabled() and dev->no_ats checks into a single
>    line in pci_ats_init()
>  - Reorder the no_ats bitfield to be after ats_stu since there is likely
>    u8-sized gap there for alignment purposes
> 
> v1: https://lore.kernel.org/linux-pci/20260223184017.688212-1-dmatlack@google.com/
> 
> Cc: Raghavendra Rao Ananta <rananta@google.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
>  drivers/pci/ats.c    |  2 +-
>  drivers/pci/quirks.c | 50 ++++++++++++++++++++++----------------------
>  include/linux/pci.h  |  1 +
>  3 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index ec6c8dbdc5e9..ceb6f5d3cb10 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -21,7 +21,7 @@ void pci_ats_init(struct pci_dev *dev)
>  {
>  	int pos;
>  
> -	if (pci_ats_disabled())
> +	if (pci_ats_disabled() || dev->no_ats)
>  		return;
>  
>  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 48946cca4be7..2c7e11830e45 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5653,7 +5653,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0422, quirk_no_ext_tags);
>  static void quirk_no_ats(struct pci_dev *pdev)
>  {
>  	pci_info(pdev, "disabling ATS\n");
> -	pdev->ats_cap = 0;
> +	pdev->no_ats = 1;
>  }
>  
>  /*
> @@ -5676,25 +5676,25 @@ static void quirk_amd_harvest_no_ats(struct pci_dev *pdev)
>  }
>  
>  /* AMD Stoney platform GPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x98e4, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x98e4, quirk_amd_harvest_no_ats);
>  /* AMD Iceland dGPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6900, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x6900, quirk_amd_harvest_no_ats);
>  /* AMD Navi10 dGPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7310, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7312, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7318, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7319, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731a, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731b, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731e, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x731f, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7310, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7312, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7318, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7319, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731a, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731b, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731e, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x731f, quirk_amd_harvest_no_ats);
>  /* AMD Navi14 dGPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7340, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7341, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7341, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7347, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x734f, quirk_amd_harvest_no_ats);
>  /* AMD Raven platform iGPU */
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x15d8, quirk_amd_harvest_no_ats);
>  
>  /*
>   * Intel IPU E2000 revisions before C0 implement incorrect endianness
> @@ -5705,15 +5705,15 @@ static void quirk_intel_e2000_no_ats(struct pci_dev *pdev)
>  	if (pdev->revision < 0x20)
>  		quirk_no_ats(pdev);
>  }
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1451, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1452, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1453, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1454, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1455, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
>  #endif /* CONFIG_PCI_ATS */
>  
>  /* Freescale PCIe doesn't support MSI in RC mode */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1c270f1d5123..850100f209e3 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -539,6 +539,7 @@ struct pci_dev {
>  	};
>  	u16		ats_cap;	/* ATS Capability offset */
>  	u8		ats_stu;	/* ATS Smallest Translation Unit */
> +	unsigned int	no_ats:1;	/* ATS disabled via quirk */
>  #endif
>  #ifdef CONFIG_PCI_PRI
>  	u16		pri_cap;	/* PRI Capability offset */
> 
> base-commit: 7df48e36313029e4c0907b2023905dd7213fd678
> -- 
> 2.53.0.1018.g2bb0e51243-goog
>