| 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
| ||
|
Message-ID: <5165448792268e184e508a9c76fa0ce382a4a389.camel@linux.ibm.com>
Date: Mon, 13 Oct 2025 11:53:55 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Nicolin Chen <nicolinc@...dia.com>, joro@...tes.org, jgg@...dia.com,
kevin.tian@...el.com
Cc: 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
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.
>
> And keep them within the group->mutex, so drivers can simply move all the
> sanity and compatibility tests from their attach_dev callbacks to the new
> test_dev callbacks without concerning about a race condition.
>
> This may be a public API someday for VFIO/IOMMUFD to run a list of attach
> tests without doing any actual attachment, which may result in a list of
> failed tests. So encourage drivers to avoid printks to prevent kernel log
> spam.
>
> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
> include/linux/iommu.h | 17 +++++--
> drivers/iommu/iommu.c | 111 ++++++++++++++++++++++++++++++------------
> 2 files changed, 93 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 801b2bd9e8d49..2ec99502dc29c 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -714,7 +714,12 @@ struct iommu_ops {
>
> /**
> * 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.
> + * 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."?
> + * It's suggested to avoid the kernel prints in this op.
> * Return:
> * * 0 - success
> * * EINVAL - can indicate that device and domain are incompatible due to
> @@ -722,11 +727,15 @@ struct iommu_ops {
> * driver shouldn't log an error, since it is legitimate for a
> * caller to test reuse of existing domains. Otherwise, it may
> * still represent some other fundamental problem
> - * * ENOMEM - out of memory
> - * * ENOSPC - non-ENOMEM type of resource allocation failures
> * * EBUSY - device is attached to a domain and cannot be changed
> * * ENODEV - device specific errors, not able to be attached
> * * <others> - treated as ENODEV by the caller. Use is discouraged
> + * @attach_dev: attach an iommu domain to a device
> + * Return:
> + * * 0 - success
> + * * ENOMEM - out of memory
> + * * ENOSPC - non-ENOMEM type of resource allocation failures
> + * * <others> - Use is discouraged
> * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The pasid of
> * the device should be left in the old config in error case.
> * @map_pages: map a physically contiguous set of pages of the same size to
> @@ -751,6 +760,8 @@ struct iommu_ops {
> * @free: Release the domain after use.
> */
> struct iommu_domain_ops {
> + int (*test_dev)(struct iommu_domain *domain, struct device *dev,
> + ioasid_t pasid, struct iommu_domain *old);
> int (*attach_dev)(struct iommu_domain *domain, struct device *dev,
> struct iommu_domain *old);
> int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
>
--- 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>
Powered by blists - more mailing lists