Messages in this thread Patch in this message | | | Date | Mon, 6 Jun 2022 18:50:33 +0100 | Subject | Re: [PATCH 2/5] iommu: Ensure device has the same iommu_ops as the domain | From | 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;
| |