[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 6 Jun 2022 18:50:33 +0100
From: Robin Murphy <robin.murphy@....com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: jgg@...dia.com, joro@...tes.org, will@...nel.org, marcan@...can.st,
sven@...npeter.dev, robdclark@...il.com, m.szyprowski@...sung.com,
krzysztof.kozlowski@...aro.org, baolu.lu@...ux.intel.com,
agross@...nel.org, bjorn.andersson@...aro.org,
matthias.bgg@...il.com, heiko@...ech.de, orsonzhai@...il.com,
baolin.wang7@...il.com, zhang.lyra@...il.com, wens@...e.org,
jernej.skrabec@...il.com, samuel@...lland.org,
jean-philippe@...aro.org, alex.williamson@...hat.com,
suravee.suthikulpanit@....com, alyssa@...enzweig.io,
alim.akhtar@...sung.com, dwmw2@...radead.org, yong.wu@...iatek.com,
mjrosato@...ux.ibm.com, gerald.schaefer@...ux.ibm.com,
thierry.reding@...il.com, vdumpa@...dia.com, jonathanh@...dia.com,
cohuck@...hat.com, iommu@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
linux-mediatek@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-s390@...r.kernel.org,
linux-sunxi@...ts.linux.dev, linux-tegra@...r.kernel.org,
virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org
Subject: Re: [PATCH 2/5] iommu: Ensure device has the same iommu_ops as the
domain
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;
Powered by blists - more mailing lists