| 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
| ||
|
Date: Tue, 20 Feb 2024 23:58:40 +0000
From: "Zhang, Tina" <tina.zhang@...el.com>
To: Zhangfei Gao <zhangfei.gao@...aro.org>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <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>, "Tian, Kevin"
<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 Zhangfei,
> -----Original Message-----
> From: Zhangfei Gao <zhangfei.gao@...aro.org>
> Sent: Wednesday, February 21, 2024 12:27 AM
> To: Zhang, Tina <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>; Tian, Kevin
> <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
Sounds like the test case tries to bind a device to a same mm multiple times without unbinding the device and the expectation is that it can always return a valid handle to pass the test. Right?
Regards,
-Tina
>
> > + 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