linux-kernel - Re: [PATCH RFC v1 1/2] 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: <aEfZlKNk4xfb41RR@nvidia.com>
Date: Tue, 10 Jun 2025 00:07:00 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>
CC: <jgg@...dia.com>, <joro@...tes.org>, <will@...nel.org>,
	<robin.murphy@....com>, <bhelgaas@...gle.com>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<patches@...ts.linux.dev>, <pjaroszynski@...dia.com>, <vsethi@...dia.com>
Subject: Re: [PATCH RFC v1 1/2] iommu: Introduce iommu_dev_reset_prepare()
 and iommu_dev_reset_done()

On Tue, Jun 10, 2025 at 12:26:07PM +0800, Baolu Lu wrote:
> On 6/10/25 02:45, Nicolin Chen wrote:
> > +	ops = dev_iommu_ops(dev);
> 
> Should this be protected by group->mutext?

Not seemingly, but should require the iommu_probe_device_lock I
think.

> > +	/*
> > +	 * group->mutex starts
> > +	 *
> > +	 * This has to hold the group mutex until the reset is done, to prevent
> > +	 * any RID or PASID domain attachment/replacement, which otherwise might
> > +	 * re-enable the ATS during the reset cycle.
> > +	 */
> > +	mutex_lock(&group->mutex);
> 
> Is it possible that group has been freed when it reaches here?

It doesn't look very obvious to me which lock we need here. But,
it's a good point that dev->iommu_group is unsafe here. Will dig
a bit later.

> > +void iommu_dev_reset_done(struct device *dev)
> > +{
> > +	struct iommu_group *group = dev->iommu_group;
> > +	const struct iommu_ops *ops;
> > +	unsigned long pasid;
> > +	void *entry;
> > +
> > +	/* Previously unlocked */
> > +	if (!dev_has_iommu(dev))
> > +		return;
> > +	ops = dev_iommu_ops(dev);
> > +	if (!ops->blocked_domain)
> > +		return;
> 
> Should it be a WARN_ON()? As proposed, reset_prepare and reset_done must
> be paired. So if reset_prepare returns failure, reset_done should not be
> called. Or not?

Yea, I agree. Should work like that.

> > +	/* group->mutex held in iommu_dev_reset_prepare() continues from here */
> > +	WARN_ON(!lockdep_is_held(&group->mutex));
> 
> Probably iommu_group_mutex_assert() and move it up?

Yes and not sure (will take another look).

> How about combining these two helpers? Something like,
> 
> int iommu_dev_block_dma_and_action(struct device *dev,
> 		int (*action)(struct pci_dev *dev))
> {
> 	prepare();
> 	action();
> 	done();
> }

That's an interesting idea! So, we wouldn't need to worry about
the pairing.

Thanks!
Nicolin