linux-kernel - RE: [PATCH v5 5/9] iommufd: Add iommufd fault object

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]

Date: Wed, 15 May 2024 07:57:28 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Jason Gunthorpe <jgg@...pe.ca>
CC: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, "Robin
 Murphy" <robin.murphy@....com>, Jean-Philippe Brucker
	<jean-philippe@...aro.org>, Nicolin Chen <nicolinc@...dia.com>, "Liu, Yi L"
	<yi.l.liu@...el.com>, Jacob Pan <jacob.jun.pan@...ux.intel.com>, "Joel
 Granados" <j.granados@...sung.com>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "virtualization@...ts.linux-foundation.org"
	<virtualization@...ts.linux-foundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 5/9] iommufd: Add iommufd fault object

> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, May 8, 2024 6:05 PM
> 
> On 2024/5/8 8:11, Jason Gunthorpe wrote:
> > On Tue, Apr 30, 2024 at 10:57:06PM +0800, Lu Baolu wrote:
> >> diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
> >> index ae65e0b85d69..1a0450a83bd0 100644
> >> --- a/drivers/iommu/iommu-priv.h
> >> +++ b/drivers/iommu/iommu-priv.h
> >> @@ -36,6 +36,10 @@ struct iommu_attach_handle {
> >>   			struct device	*dev;
> >>   			refcount_t	users;
> >>   		};
> >> +		/* attach data for IOMMUFD */
> >> +		struct {
> >> +			void		*idev;
> >> +		};
> > We can use a proper type here, just forward declare it.
> >
> > But this sequence in the other patch:
> >
> > +       ret = iommu_attach_group(hwpt->domain, idev->igroup->group);
> > +       if (ret) {
> > +               iommufd_fault_iopf_disable(idev);
> > +               return ret;
> > +       }
> > +
> > +       handle = iommu_attach_handle_get(idev->igroup->group,
> IOMMU_NO_PASID, 0);
> > +       handle->idev = idev;
> >
> > Is why I was imagining the caller would allocate, because now we have
> > the issue that a fault capable domain was installed into the IOMMU
> > before it's handle could be fully setup, so we have a race where a
> > fault could come in right between those things. Then what happens?
> > I suppose we can retry the fault and by the time it comes back the
> > race should resolve. A bit ugly I suppose.
> 
> You are right. It makes more sense if the attached data is allocated and
> managed by the caller. I will go in this direction and update my series.
> I will also consider other review comments you have given in other
> places.
> 

Does this direction imply a new iommu_attach_group_handle() helper
to pass in the caller-allocated handle pointer or exposing a new
iommu_group_set_handle() to set the handle to the group pasid_array 
and then having iomm_attach_group() to update the domain info in
the handle?