RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jacob Pan <jacob.jun.pan@linux.intel.com>,
	Alex Williamson <alex.williamson@redhat.com>
Cc: "Raj, Ashok" <ashok.raj@intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	David Woodhouse <dwmw2@infradead.org>
Subject: RE: [PATCH v2 1/3] iommu/uapi: Define uapi version and capabilities
Date: Tue, 14 Apr 2020 23:47:40 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D19D81F901@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20200414153226.30d8cab3@jacob-builder>

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, April 15, 2020 6:32 AM
> 
> On Tue, 14 Apr 2020 10:13:04 -0700
> Jacob Pan <jacob.jun.pan@linux.intel.com> wrote:
> 
> > > > >  In any of the proposed solutions, the
> > > > > IOMMU driver is ultimately responsible for validating the user
> > > > > data, so do we want vfio performing the copy_from_user() to an
> > > > > object that could later be assumed to be sanitized, or should
> > > > > vfio just pass a user pointer to make it obvious that the
> > > > > consumer is responsible for all the user protections?  Seems
> > > > > like the latter.
> > > > I like the latter as well.
> > > >
> On a second thought, I think the former is better. Two reasons:
> 
> 1. IOMMU API such as page_response is also used in baremetal. So it is
> not suitable to pass a __user *.
> https://www.spinics.net/lists/arm-kernel/msg798677.html

You can have a wrapped version accepting a __user* and an internal
version for kernel pointers.

> 
> 2. Some data are in the mandatory (fixed offset, never removed or
> extended) portion of the uAPI structure. It is simpler for VFIO to
> extract that and pass it to IOMMU API. For example, the PASID value used
> for unbind_gpasid(). VFIO also need to sanitize the PASID value to make
> sure it belongs to the same VM that did the allocation.

I don't think this makes much difference. If anyway you still plan to
let IOMMU driver parse some user pointers, why not making a clear
split to have it sparse all IOMMU specific fields?

Thanks
Kevin

> 
> 
> > > > >  That still really
> > > > > doesn't address what's in that user data blob yet, but the vfio
> > > > > interface could be:
> > > > >
> > > > > struct {
> > > > > 	__u32 argsz;
> > > > > 	__u32 flags;
> > > > > 	__u8  data[];
> > > > > }
> > > > >
> > > > > Where flags might be partitioned like we do for DEVICE_FEATURE
> > > > > to indicate the format of data and what vfio should do with it,
> > > > > and data might simply be defined as a (__u64 __user *).
> > > > >
> > > > So, __user * will be passed to IOMMU driver if VFIO checks minsz
> > > > include flags and they are valid.
> > > > IOMMU driver can copy the rest based on the mandatory
> > > > version/minsz and flags in the IOMMU uAPI structs.
> > > > Does it sound right? This is really choice #2.
> > >
> > > Sounds like each IOMMU UAPI struct just needs to have an embedded
> > > size and flags field, but yes.
> > >
> > Yes, an argsz field can be added to each UAPI. There are already flags
> > or the equivalent. IOMMU driver can process the __user * based on the
> > argsz, flags, check argsz against offsetofend(iommu_uapi_struct,
> > last_element), etc.;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu