| 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
| ||
|
Message-ID: <e51496f4-2874-04ad-818f-e4579cf3a1d3@linux.intel.com>
Date: Tue, 3 Jan 2023 10:45:24 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
Christoph Hellwig <hch@...radead.org>,
Kevin Tian <kevin.tian@...el.com>,
Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Jean-Philippe Brucker <jean-philippe@...aro.org>,
Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
Hector Martin <marcan@...can.st>,
Sven Peter <sven@...npeter.dev>,
Rob Clark <robdclark@...il.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Yong Wu <yong.wu@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Heiko Stuebner <heiko@...ech.de>,
Matthew Rosato <mjrosato@...ux.ibm.com>,
Orson Zhai <orsonzhai@...il.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
Chunyan Zhang <zhang.lyra@...il.com>,
Chen-Yu Tsai <wens@...e.org>,
Thierry Reding <thierry.reding@...il.com>,
iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 18/20] iommu: Call set_platform_dma if default domain
is unavailable
Hi Jason,
On 11/28/22 10:57 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote:
>> If the IOMMU driver has no default domain support, call set_platform_dma
>> explicitly to return the kernel DMA control back to the platform DMA ops.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> ---
>> drivers/iommu/iommu.c | 28 ++++++++--------------------
>> 1 file changed, 8 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 7c99d8eb3182..e4966f088184 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2040,16 +2040,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>> return 0;
>> }
>>
>> -static void __iommu_detach_device(struct iommu_domain *domain,
>> - struct device *dev)
>> -{
>> - if (iommu_is_attach_deferred(dev))
>> - return;
>
> This removal might want to be its own patch with an explanation.
>
> It looks like at the current moment __iommu_detach_device() is only
> called via call chains that are after the device driver is attached -
> eg via explicit attach APIs called by the device driver.
>
> So it should just unconditionally work. It is actually looks like a
> bug that we were blocking detach on these paths since the attach was
> unconditional and the caller is going to free the (probably) UNAMANGED
> domain once this returns.
>
> The only place we should be testing for deferred attach is during the
> initial point the dma device is linked to the group, and then again
> during the dma api calls to check if the device
>
> This maybe the patch that is needed to explain this:
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d69ebba81bebd8..06f1fe6563bb30 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -993,8 +993,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>
> mutex_lock(&group->mutex);
> list_add_tail(&device->list, &group->devices);
> - if (group->domain && !iommu_is_attach_deferred(dev))
> - ret = __iommu_attach_device(group->domain, dev);
> + if (group->domain)
> + ret = iommu_group_do_dma_first_attach(dev, group->domain);
> mutex_unlock(&group->mutex);
> if (ret)
> goto err_put_group;
> @@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>
> }
>
> -static int iommu_group_do_dma_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> {
> struct iommu_domain *domain = data;
> - int ret = 0;
>
> - if (!iommu_is_attach_deferred(dev))
> - ret = __iommu_attach_device(domain, dev);
> + lockdep_assert_held(&dev->iommu_group->mutex);
>
> - return ret;
> + if (iommu_is_attach_deferred(dev)) {
> + dev->iommu->attach_deferred = 1;
> + return 0;
> + }
> +
> + return __iommu_attach_device(domain, dev);
> }
>
> -static int __iommu_group_dma_attach(struct iommu_group *group)
> +static int __iommu_group_dma_first_attach(struct iommu_group *group)
> {
> return __iommu_group_for_each_dev(group, group->default_domain,
> - iommu_group_do_dma_attach);
> + iommu_group_do_dma_first_attach);
> }
>
> static int iommu_group_do_probe_finalize(struct device *dev, void *data)
> @@ -1839,7 +1842,7 @@ int bus_iommu_probe(struct bus_type *bus)
>
> iommu_group_create_direct_mappings(group);
>
> - ret = __iommu_group_dma_attach(group);
> + ret = __iommu_group_dma_first_attach(group);
>
> mutex_unlock(&group->mutex);
>
> @@ -1971,9 +1974,11 @@ static int __iommu_attach_device(struct iommu_domain *domain,
> return -ENODEV;
>
> ret = domain->ops->attach_dev(domain, dev);
> - if (!ret)
> - trace_attach_device_to_domain(dev);
> - return ret;
> + if (ret)
> + return ret;
> + dev->iommu->attach_deferred = 0;
> + trace_attach_device_to_domain(dev);
> + return 0;
> }
>
> /**
> @@ -2018,7 +2023,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
>
> int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
> {
> - if (iommu_is_attach_deferred(dev))
> + if (dev->iommu && dev->iommu->attach_deferred)
> return __iommu_attach_device(domain, dev);
>
> return 0;
> @@ -2027,9 +2032,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
> static void __iommu_detach_device(struct iommu_domain *domain,
> struct device *dev)
> {
> - if (iommu_is_attach_deferred(dev))
> - return;
> -
> domain->ops->detach_dev(domain, dev);
> trace_detach_device_from_domain(dev);
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 1690c334e51631..ebac04a13fff68 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -413,6 +413,7 @@ struct dev_iommu {
> struct iommu_device *iommu_dev;
> void *priv;
> u32 max_pasids;
> + u8 attach_deferred;
> };
>
> int iommu_device_register(struct iommu_device *iommu,
Thanks for the patch! It seems that we also need to call
iommu_group_do_dma_first_attach() in the iommu_probe_device() path?
@@ -401,7 +425,7 @@ int iommu_probe_device(struct device *dev)
* attach the default domain.
*/
if (group->default_domain && !group->owner) {
- ret = __iommu_attach_device(group->default_domain, dev);
+ ret = iommu_group_do_dma_first_attach(dev,
group->default_domain);
if (ret) {
mutex_unlock(&group->mutex);
iommu_group_put(group);
By the way, I'd like to put above code in a separated patch of the next
version, can I add your signed-off-by?
--
Best regards,
baolu
Powered by blists - more mailing lists