| 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: <CABQgh9GWcqUeBkHQCpj5tzu6FnEgpOp3KOQ6s9c0X0KU7Ov1qw@mail.gmail.com>
Date: Wed, 21 Feb 2024 00:26:56 +0800
From: Zhangfei Gao <zhangfei.gao@...aro.org>
To: Tina Zhang <tina.zhang@...el.com>
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
David Woodhouse <dwmw2@...radead.org>, Lu Baolu <baolu.lu@...ux.intel.com>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
Jason Gunthorpe <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>, Nicolin Chen <nicolinc@...dia.com>,
Michael Shavit <mshavit@...gle.com>, Vasant Hegde <vasant.hegde@....com>,
Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains
Hi, Tina
On Fri, 27 Oct 2023 at 08:06, Tina Zhang <tina.zhang@...el.com> wrote:
>
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when no one is using it (the reference count is decremented
> in iommu_sva_unbind_device()). However, although sva domains and their
> PASID are separate objects such that their own life cycles could be
> handled independently, an enqcmd use case may require releasing the
> PASID in releasing the mm (i.e., once a PASID is allocated for a mm, it
> will be permanently used by the mm and won't be released until the end
> of mm) and only allows to drop the PASID after the sva domains are
> released. To this end, mmgrab() is called in iommu_sva_domain_alloc() to
> increment the mm reference count and mmdrop() is invoked in
> iommu_domain_free() to decrement the mm reference count.
>
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
>
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
>
> Reviewed-by: Lu Baolu <baolu.lu@...ux.intel.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@....com>
> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> Tested-by: Nicolin Chen <nicolinc@...dia.com>
> Signed-off-by: Tina Zhang <tina.zhang@...el.com>
> Signed-off-by: Jason Gunthorpe <jgg@...dia.com>
> ---
> drivers/iommu/iommu-sva.c | 92 +++++++++++++++++++++++----------------
> include/linux/iommu.h | 23 ++++++++--
> 2 files changed, 74 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 4a2f5699747f..5175e8d85247 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -12,32 +12,42 @@
> static DEFINE_MUTEX(iommu_sva_lock);
>
> /* Allocate a PASID for the mm within range (inclusive) */
> -static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
> +static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct device *dev)
> {
> + struct iommu_mm_data *iommu_mm;
> ioasid_t pasid;
> - int ret = 0;
> +
> + lockdep_assert_held(&iommu_sva_lock);
>
> if (!arch_pgtable_dma_compat(mm))
> - return -EBUSY;
> + return ERR_PTR(-EBUSY);
>
> - mutex_lock(&iommu_sva_lock);
> + iommu_mm = mm->iommu_mm;
> /* Is a PASID already associated with this mm? */
> - if (mm_valid_pasid(mm)) {
> - if (mm->pasid >= dev->iommu->max_pasids)
> - ret = -EOVERFLOW;
> - goto out;
> + if (iommu_mm) {
> + if (iommu_mm->pasid >= dev->iommu->max_pasids)
> + return ERR_PTR(-EOVERFLOW);
> + return iommu_mm;
> }
>
> + iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> + if (!iommu_mm)
> + return ERR_PTR(-ENOMEM);
> +
> pasid = iommu_alloc_global_pasid(dev);
> if (pasid == IOMMU_PASID_INVALID) {
> - ret = -ENOSPC;
> - goto out;
> + kfree(iommu_mm);
> + return ERR_PTR(-ENOSPC);
> }
> - mm->pasid = pasid;
> - ret = 0;
> -out:
> - mutex_unlock(&iommu_sva_lock);
> - return ret;
> + iommu_mm->pasid = pasid;
> + INIT_LIST_HEAD(&iommu_mm->sva_domains);
> + /*
> + * Make sure the write to mm->iommu_mm is not reordered in front of
> + * initialization to iommu_mm fields. If it does, readers may see a
> + * valid iommu_mm with uninitialized values.
> + */
> + smp_store_release(&mm->iommu_mm, iommu_mm);
> + return iommu_mm;
> }
>
> /**
> @@ -58,31 +68,33 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
> */
> struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
> {
> + struct iommu_mm_data *iommu_mm;
> struct iommu_domain *domain;
> struct iommu_sva *handle;
> int ret;
>
> + mutex_lock(&iommu_sva_lock);
> +
> /* Allocate mm->pasid if necessary. */
> - ret = iommu_sva_alloc_pasid(mm, dev);
> - if (ret)
> - return ERR_PTR(ret);
> + iommu_mm = iommu_alloc_mm_data(mm, dev);
> + if (IS_ERR(iommu_mm)) {
> + ret = PTR_ERR(iommu_mm);
> + goto out_unlock;
> + }
>
> handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> - if (!handle)
> - return ERR_PTR(-ENOMEM);
> -
> - mutex_lock(&iommu_sva_lock);
> - /* Search for an existing domain. */
> - domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
> - IOMMU_DOMAIN_SVA);
> - if (IS_ERR(domain)) {
> - ret = PTR_ERR(domain);
> + if (!handle) {
> + ret = -ENOMEM;
> goto out_unlock;
> }
>
> - if (domain) {
> - domain->users++;
> - goto out;
Our multi bind test case broke since 6.8-rc1.
The test case can use same domain & pasid, return different handle,
6.7 simply domain->users ++ and return.
> + /* Search for an existing domain. */
> + list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
> + ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid);
Now iommu_attach_device_pasid return BUSY since the same pasid.
And then iommu_sva_bind_device attach ret=-16
> + if (!ret) {
Simply tried if (!ret || ret == -EBUSY)
The test passes, but report waring
WARNING: CPU: 12 PID: 2992 at drivers/iommu/iommu.c:3591
iommu_detach_device_pasid+0xa4/0xd0
Will check more tomorrow.
> + domain->users++;
> + goto out;
> + }
> }
>
Thanks
Powered by blists - more mailing lists