| 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: <BN9PR11MB52769385E96377F5FD8CE2928CAD9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 15 Jun 2022 06:13:17 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>,
Joerg Roedel <joro@...tes.org>,
"Raj, Ashok" <ashok.raj@...el.com>,
Christoph Hellwig <hch@...radead.org>,
"Jason Gunthorpe" <jgg@...dia.com>
CC: Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
"Liu, Yi L" <yi.l.liu@...el.com>,
"Pan, Jacob jun" <jacob.jun.pan@...el.com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 01/12] iommu/vt-d: debugfs: Remove device_domain_lock
usage
> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, June 15, 2022 9:54 AM
>
> On 2022/6/14 14:43, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@...ux.intel.com>
> >> Sent: Tuesday, June 14, 2022 10:51 AM
> >>
> >> The domain_translation_struct debugfs node is used to dump the DMAR
> >> page
> >> tables for the PCI devices. It potentially races with setting domains to
> >> devices. The existing code uses a global spinlock device_domain_lock to
> >> avoid the races, but this is problematical as this lock is only used to
> >> protect the device tracking lists of each domain.
> > is it really problematic at this point? Before following patches are applied
> > using device_domain_lock should have similar effect as holding the group
> > lock.
> >
> > Here it might make more sense to just focus on removing the use of
> > device_domain_lock outside of iommu.c. Just that using group lock is
> > cleaner and more compatible to following cleanups.
> >
> > and it's worth mentioning that racing with page table updates is out
> > of the scope of this series. Probably also add a comment in the code
> > to clarify this point.
> >
>
> Hi Kevin,
>
> How do you like below updated patch?
Yes, this is better.
>
> From cecc9a0623780a11c4ea4d0a15aa6187f01541c4 Mon Sep 17 00:00:00
> 2001
> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Date: Sun, 29 May 2022 10:18:56 +0800
> Subject: [PATCH 1/1] iommu/vt-d: debugfs: Remove device_domain_lock
> usage
>
> The domain_translation_struct debugfs node is used to dump the DMAR
> page
> tables for the PCI devices. It potentially races with setting domains to
> devices. The existing code uses the global spinlock device_domain_lock to
> avoid the races.
>
> This removes the use of device_domain_lock outside of iommu.c by replacing
> it with the group mutex lock. Using the group mutex lock is cleaner and
> more compatible to following cleanups.
>
> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> ---
> drivers/iommu/intel/debugfs.c | 42 +++++++++++++++++++++++++----------
> drivers/iommu/intel/iommu.c | 2 +-
> drivers/iommu/intel/iommu.h | 1 -
> 3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
> index d927ef10641b..f4acd8993f60 100644
> --- a/drivers/iommu/intel/debugfs.c
> +++ b/drivers/iommu/intel/debugfs.c
> @@ -342,13 +342,13 @@ static void pgtable_walk_level(struct seq_file *m,
> struct dma_pte *pde,
> }
> }
>
> -static int show_device_domain_translation(struct device *dev, void *data)
> +static int __show_device_domain_translation(struct device *dev, void *data)
> {
> - struct device_domain_info *info = dev_iommu_priv_get(dev);
> - struct dmar_domain *domain = info->domain;
> + struct dmar_domain *domain;
> struct seq_file *m = data;
> u64 path[6] = { 0 };
>
> + domain = to_dmar_domain(iommu_get_domain_for_dev(dev));
> if (!domain)
> return 0;
>
> @@ -359,20 +359,38 @@ static int show_device_domain_translation(struct
> device *dev, void *data)
> pgtable_walk_level(m, domain->pgd, domain->agaw + 2, 0, path);
> seq_putc(m, '\n');
>
> - return 0;
> + return 1;
> }
>
> -static int domain_translation_struct_show(struct seq_file *m, void *unused)
> +static int show_device_domain_translation(struct device *dev, void *data)
> {
> - unsigned long flags;
> - int ret;
> + struct iommu_group *group;
>
> - spin_lock_irqsave(&device_domain_lock, flags);
> - ret = bus_for_each_dev(&pci_bus_type, NULL, m,
> - show_device_domain_translation);
> - spin_unlock_irqrestore(&device_domain_lock, flags);
> + group = iommu_group_get(dev);
> + if (group) {
> + /*
> + * The group->mutex is held across the callback, which will
> + * block calls to iommu_attach/detach_group/device. Hence,
> + * the domain of the device will not change during traversal.
> + *
> + * All devices in an iommu group share a single domain,
> hence
> + * we only dump the domain of the first device. Even though,
bus_for_each_dev() will still lead to duplicated dump in the same group
but probably we can leave with it for a debug interface.
> + * this code still possibly races with the iommu_unmap()
> + * interface. This could be solved by RCU-freeing the page
> + * table pages in the iommu_unmap() path.
> + */
> + iommu_group_for_each_dev(group, data,
> + __show_device_domain_translation);
> + iommu_group_put(group);
> + }
>
> - return ret;
> + return 0;
> +}
> +
> +static int domain_translation_struct_show(struct seq_file *m, void *unused)
> +{
> + return bus_for_each_dev(&pci_bus_type, NULL, m,
> + show_device_domain_translation);
> }
> DEFINE_SHOW_ATTRIBUTE(domain_translation_struct);
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 19024dc52735..a39d72a9d1cf 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -314,7 +314,7 @@ static int iommu_skip_te_disable;
> #define IDENTMAP_GFX 2
> #define IDENTMAP_AZALIA 4
>
> -DEFINE_SPINLOCK(device_domain_lock);
> +static DEFINE_SPINLOCK(device_domain_lock);
> static LIST_HEAD(device_domain_list);
>
> /*
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index a22adfbdf870..8a6d64d726c0 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -480,7 +480,6 @@ enum {
> #define VTD_FLAG_SVM_CAPABLE (1 << 2)
>
> extern int intel_iommu_sm;
> -extern spinlock_t device_domain_lock;
>
> #define sm_supported(iommu) (intel_iommu_sm &&
> ecap_smts((iommu)->ecap))
> #define pasid_supported(iommu) (sm_supported(iommu) &&
> \
> --
> 2.25.1
>
> Best regards,
> baolu
Powered by blists - more mailing lists