Re: [PATCH 2/5] iommu: Ensure device has the same iommu_ops as the domain

[Robin Murphy]

On 2022-06-06 17:51, Nicolin Chen wrote:
> Hi Robin,
>
> On Mon, Jun 06, 2022 at 03:33:42PM +0100, Robin Murphy wrote:
> > On 2022-06-06 07:19, Nicolin Chen wrote:
> > > The core code should not call an iommu driver op with a struct device
> > > parameter unless it knows that the dev_iommu_priv_get() for that struct
> > > device was setup by the same driver. Otherwise in a mixed driver system
> > > the iommu_priv could be casted to the wrong type.
> >
> > We don't have mixed-driver systems, and there are plenty more
> > significant problems than this one to solve before we can (but thanks
> > for pointing it out - I hadn't got as far as auditing the public
> > interfaces yet). Once domains are allocated via a particular device's
> > IOMMU instance in the first place, there will be ample opportunity for
> > the core to stash suitable identifying information in the domain for
> > itself. TBH even the current code could do it without needing the
> > weirdly invasive changes here.
>
> Do you have an alternative and less invasive solution in mind?
>
> > > Store the iommu_ops pointer in the iommu_domain and use it as a check to
> > > validate that the struct device is correct before invoking any domain op
> > > that accepts a struct device.
> >
> > In fact this even describes exactly that - "Store the iommu_ops pointer
> > in the iommu_domain", vs. the "Store the iommu_ops pointer in the
> > iommu_domain_ops" which the patch is actually doing :/
>
> Will fix that.

Well, as before I'd prefer to make the code match the commit message - 
if I really need to spell it out, see below - since I can't imagine that 
we should ever have need to identify a set of iommu_domain_ops in 
isolation, therefore I think it's considerably clearer to use the 
iommu_domain itself. However, either way we really don't need this yet, 
so we may as well just go ahead and remove the redundant test from VFIO 
anyway, and I can add some form of this patch to my dev branch for now.

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cde2e1d6ab9b..72990edc9314 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1902,6 +1902,7 @@ static struct iommu_domain 
*__iommu_domain_alloc(struct device *dev,
 	domain->type = type;
 	/* Assume all sizes by default; the driver may override this later */
 	domain->pgsize_bitmap = ops->pgsize_bitmap;
+	domain->owner = ops;
 	if (!domain->ops)
 		domain->ops = ops->default_domain_ops;

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6f64cbbc6721..79e557207f53 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -89,6 +89,7 @@ struct iommu_domain_geometry {

 struct iommu_domain {
 	unsigned type;
+	const struct iommu_ops *owner; /* Who allocated this domain */
 	const struct iommu_domain_ops *ops;
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	iommu_fault_handler_t handler;