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] [day] [month] [year] [list]
Message-ID: <aO01TkkA7ZLDgqXg@Asurada-Nvidia>
Date: Mon, 13 Oct 2025 10:22:22 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Niklas Schnelle <schnelle@...ux.ibm.com>
CC: <joro@...tes.org>, <jgg@...dia.com>, <kevin.tian@...el.com>,
	<suravee.suthikulpanit@....com>, <will@...nel.org>, <robin.murphy@....com>,
	<sven@...nel.org>, <j@...nau.net>, <jean-philippe@...aro.org>,
	<robin.clark@....qualcomm.com>, <dwmw2@...radead.org>,
	<baolu.lu@...ux.intel.com>, <yong.wu@...iatek.com>, <matthias.bgg@...il.com>,
	<angelogioacchino.delregno@...labora.com>, <tjeznach@...osinc.com>,
	<pjw@...nel.org>, <palmer@...belt.com>, <aou@...s.berkeley.edu>,
	<heiko@...ech.de>, <mjrosato@...ux.ibm.com>, <wens@...e.org>,
	<jernej.skrabec@...il.com>, <samuel@...lland.org>,
	<thierry.reding@...il.com>, <jonathanh@...dia.com>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <asahi@...ts.linux.dev>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-arm-msm@...r.kernel.org>,
	<linux-mediatek@...ts.infradead.org>, <linux-riscv@...ts.infradead.org>,
	<linux-rockchip@...ts.infradead.org>, <linux-s390@...r.kernel.org>,
	<linux-sunxi@...ts.linux.dev>, <linux-tegra@...r.kernel.org>,
	<virtualization@...ts.linux.dev>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v1 02/20] iommu: Introduce a test_dev domain op and an
 internal helper

Hi Niklas,

On Mon, Oct 13, 2025 at 11:53:55AM +0200, Niklas Schnelle wrote:
> On Sun, 2025-10-12 at 17:04 -0700, Nicolin Chen wrote:
> > Add a new test_dev domain op for driver to test the compatibility between
> > a domain and a device at the driver level, before calling into the actual
> > attachment/replacement of a domain. Support pasid for set_dev_pasid call.
> > 
> > Move existing core-level compatibility tests to a helper function. Invoke
> > it prior to:
> >  * __iommu_attach_device() or its wrapper __iommu_device_set_domain()
> >  * __iommu_set_group_pasid()
> 
> Should this list also include iommu_deferred_attach()? The code does
> include it.

iommu_deferred_attach() invokes __iommu_attach_device(), so it is
already included in the list :)

> >  /**
> >   * struct iommu_domain_ops - domain specific operations
> > - * @attach_dev: attach an iommu domain to a device
> > + * @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid call.
> > + *            A driver-level callback of this op should do a thorough sanity, to
> 
> You're missing the word "check" above.

Ack.

> > + *            make sure a device is compatible with the domain. So the following
> > + *            @attach_dev and @set_dev_pasid functions would likely succeed with
> > + *            only one exception due to a temporary failure like out of memory.
> 
> Nit: "… only one exception …" / "… like out of memory …" this sounds a
> bit odd to me because on the one hand it's one exception but then also
> a group (temporary failures).
> 
> Maybe better:
> "… would likely succeed with only the exception of temporary failures
> like out of memory."?

Sure. I can do that. Fixing both parts, it would be:

 * @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid call.
 *            A driver callback of this op should do a thorough sanity check, to
 *            make sure a device is compatible with the domain, so the following
 *            @attach_dev and @set_dev_pasid functions would likely succeed with
 *            only the exception of temporary failures like out of memory.

> --- snip ---
> > @@ -3615,6 +3657,11 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
> >  	ret = 0;
> >  
> >  	if (curr_domain != domain) {
> > +		ret = __iommu_domain_test_device(domain, dev, pasid,
> > +						 curr_domain);
> > +		if (ret)
> > +			goto out_unlock;
> > +
> >  		ret = __iommu_set_group_pasid(domain, group,
> >  					      pasid, curr_domain);
> >  		if (ret)
> 
> Apart from the comment and commit description nits mentioned above this
> looks good to me.
> 
> Reviewed-by: Niklas Schnelle <schnelle@...ux.ibm.com>

Thanks for the review!
Nicolin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ