linux-kernel - Re: [PATCH RFC v2 3/4] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIfKjyvMeIYac23A@Asurada-Nvidia>
Date: Mon, 28 Jul 2025 12:07:59 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <joro@...tes.org>, <will@...nel.org>, <robin.murphy@....com>,
	<rafael@...nel.org>, <lenb@...nel.org>, <bhelgaas@...gle.com>,
	<iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<linux-acpi@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<patches@...ts.linux.dev>, <pjaroszynski@...dia.com>, <vsethi@...dia.com>,
	<helgaas@...nel.org>, <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH RFC v2 3/4] iommu: Introduce iommu_dev_reset_prepare()
 and iommu_dev_reset_done()

On Sun, Jul 27, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 22, 2025 at 02:58:21PM -0700, Nicolin Chen wrote:
> > > /*
> > >  * This is called on the dma mapping fast path so avoid locking. This
> > >  * is racy, but we have an expectation that the driver will setup its
> > >  * DMAs inside probe while still single threaded to avoid racing.
> > >  */
> > > if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))
> > 
> > This triggers a build error as attach_deferred is a bit-field. So I
> > am changing it from "u32 attach_deferred:1" to "bool" for this.
> 
> Bleck, that seems undesirable.

But inevitable for READ_ONCE :(

> > And, to keep the original logic, I think it should be:
> > 	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))
> 
> That doesn't seem right, if there is no iommu by the time a driver is
> probed there never will be an iommu and this device should be running
> in direct mode only.

Well, the current function does:
	if (dev->iommu && dev->iommu->attach_deferred)
		return __iommu_attach_device(domain, dev);
	return 0;

So, matching to that logic, it would be:
	if (!dev->iommu || !dev->iommu->attach_deferred)
		return 0;
	return __iommu_attach_device(domain, dev);
then add guard(mutex).

I do see your point. Yet, given that it is an exported function,
I think it'd be safer to have a check. Perhaps it should give a
WARN_ON(!dev->iommu).

> > > And of course it is already quite crazy to be doing FLR during a
> > > device probe so this is not a realistic scenario.
> > 
> > Hmm, I am not sure about that, as I see iommu_deferred_attach() get
> > mostly invoked by a dma_alloc() or even a dma_map(). So, this might
> > not be confined to a device probe?
> 
> Once you do deferred_attach the first time it is done and won't have
> any further impact. So long as the dev->iommu->attach_deferred guards
> any changes to domains it is unlikely to be racing with FLR.

I see. The existing callers are all in dma-iommu.c. So, we can
assume that iommu_deferred_attach() is already done, when a PCI
driver calls any function from dma-iommu.c.

> > > Either ignore this condition with the rational that we are about to
> > > reset it so it doesn't matter, or we need to establish a new paging
> > > domain for isolation purposes that has the RMR setup.
> > 
> > Ah, you are right. ARM MSI in a VM uses RMR and sets this.
> > 
> > But does it also raise a question that a VM having RMR can't use
> > the blocked_domain, as __iommu_device_set_domain() has the exact
> > same check rejecting blocked_domain? Not sure if there would be
> > some unintended consequnce though...
> 
> Sounds like it needs some sorting out.. For the purposes of FLR I
> think the blocked domain is OK, so maybe just move some of those
> checks around?

These two new APIs call the lower-level __iommu_attach_device()
that does not check require_direct. So, we are fine, so long as
we don't check it in the new API as you previously pointed out.

I'm worried about using blocked domains in general.

Thanks
Nicolin