linux-kernel - Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains

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: <CABQgh9FD2fup3awxNyu3LpQcB6f-HF5eb1B4fhVHxLJ0O76oVQ@mail.gmail.com>
Date: Wed, 21 Feb 2024 14:26:49 +0800
From: Zhangfei Gao <zhangfei.gao@...aro.org>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: "Zhang, Tina" <tina.zhang@...el.com>, "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, David Woodhouse <dwmw2@...radead.org>, 
	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>, Jean-Philippe Brucker <jean-philippe@...aro.org>, 
	Hao Fang <fanghao11@...wei.com>
Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains

On Wed, 21 Feb 2024 at 11:52, Baolu Lu <baolu.lu@...ux.intel.com> wrote:
>
> On 2024/2/21 10:45, Zhangfei Gao wrote:
> > On Wed, 21 Feb 2024 at 10:06, Baolu Lu<baolu.lu@...ux.intel.com>
> > wrote:
> >> On 2024/2/21 9:28, Zhangfei Gao wrote:
> >>> On Wed, 21 Feb 2024 at 07:58, Zhang, Tina<tina.zhang@...el.com>
> >>> wrote:
> >>>
> >>>>>> 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?
> >>> Yes
> >>>
> >>> The device can bind to the same mm multi-times and return
> >>> different handle, Since the refcount, no need to unbind and bind
> >>> sequently, The unbind can happen later with the handle.
> >> Is there any real use case to bind an mm to the pasid of a device
> >> multiple times? If there are cases, is it better to handle this in
> >> the uacce driver?
> > Yes, it is required for multi-thread, the device can provide
> > multi-queue to speed up.
> >
> >> From iommu core's perspective, it doesn't make sense to attach the
> >> same domain to the same device (or pasid) multiple times.
> > But is it the refcount domain->user++ used for? Is there any reason
> > not doing this.
>
> I was just thinking about whether to do this in the iommu core, or in
> the upper layers, like uacce or iommufd. It seems that there is no need
> to attach a domain to a device or pasid again if it has already been
> attached.

It would be more complicated since the return handle can be used to
distinguish different queues of the device.

I think domain->user should handle this case as before.

Anyway, I have sent a patch to get more feedback.

Thanks