Re: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
On 2022/6/2 14:29, Tian, Kevin wrote:
From: Baolu Lu
Sent: Wednesday, June 1, 2022 7:02 PM
On 2022/6/1 17:28, Tian, Kevin wrote:
From: Lu Baolu
Sent: Friday, May 27, 2022 2:30 PM
When the IOMMU domain is about to be freed, it should not be set on
any
device. Instead of silently dealing with some bug cases, it's better to
trigger a warning to report and fix any potential bugs at the first time.
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains
*/
- domain_remove_dev_info(domain);
+ if (WARN_ON(!list_empty(>devices)))
+ return;
warning is good but it doesn't mean the driver shouldn't deal with
that situation to make it safer e.g. blocking DMA from all attached
device...
I have ever thought the same thing. :-)
Blocking DMA from attached device should be done when setting blocking
domain to the device. It should not be part of freeing a domain.
yes but here we are talking about some bug scenario.
Here, the caller asks the driver to free the domain, but the driver
finds that something is wrong. Therefore, it warns and returns directly.
The domain will still be there in use until the next set_domain().
at least it'd look safer if we always try to unmap the entire domain i.e.:
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains */
- domain_remove_dev_info(domain);
if (domain->pgd) {
LIST_HEAD(freelist);
domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw), );
put_pages_list();
}
+ if (WARN_ON(!list_empty(>devices)))
+ return;
kfree(domain);
}
Fair enough. Removing all mappings is safer.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
> From: Baolu Lu
> Sent: Wednesday, June 1, 2022 7:02 PM
>
> On 2022/6/1 17:28, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> Sent: Friday, May 27, 2022 2:30 PM
> >>
> >> When the IOMMU domain is about to be freed, it should not be set on
> any
> >> device. Instead of silently dealing with some bug cases, it's better to
> >> trigger a warning to report and fix any potential bugs at the first time.
> >>
> >
> >
> >> static void domain_exit(struct dmar_domain *domain)
> >> {
> >> -
> >> - /* Remove associated devices and clear attached or cached domains
> >> */
> >> - domain_remove_dev_info(domain);
> >> + if (WARN_ON(!list_empty(>devices)))
> >> + return;
> >>
> >
> > warning is good but it doesn't mean the driver shouldn't deal with
> > that situation to make it safer e.g. blocking DMA from all attached
> > device...
>
> I have ever thought the same thing. :-)
>
> Blocking DMA from attached device should be done when setting blocking
> domain to the device. It should not be part of freeing a domain.
yes but here we are talking about some bug scenario.
>
> Here, the caller asks the driver to free the domain, but the driver
> finds that something is wrong. Therefore, it warns and returns directly.
> The domain will still be there in use until the next set_domain().
>
at least it'd look safer if we always try to unmap the entire domain i.e.:
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains */
- domain_remove_dev_info(domain);
if (domain->pgd) {
LIST_HEAD(freelist);
domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw), );
put_pages_list();
}
+ if (WARN_ON(!list_empty(>devices)))
+ return;
kfree(domain);
}
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
On 2022/6/1 17:28, Tian, Kevin wrote:
From: Lu Baolu
Sent: Friday, May 27, 2022 2:30 PM
When the IOMMU domain is about to be freed, it should not be set on any
device. Instead of silently dealing with some bug cases, it's better to
trigger a warning to report and fix any potential bugs at the first time.
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains
*/
- domain_remove_dev_info(domain);
+ if (WARN_ON(!list_empty(>devices)))
+ return;
warning is good but it doesn't mean the driver shouldn't deal with
that situation to make it safer e.g. blocking DMA from all attached
device...
I have ever thought the same thing. :-)
Blocking DMA from attached device should be done when setting blocking
domain to the device. It should not be part of freeing a domain.
Here, the caller asks the driver to free the domain, but the driver
finds that something is wrong. Therefore, it warns and returns directly.
The domain will still be there in use until the next set_domain().
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
> From: Lu Baolu
> Sent: Friday, May 27, 2022 2:30 PM
>
> When the IOMMU domain is about to be freed, it should not be set on any
> device. Instead of silently dealing with some bug cases, it's better to
> trigger a warning to report and fix any potential bugs at the first time.
>
> static void domain_exit(struct dmar_domain *domain)
> {
> -
> - /* Remove associated devices and clear attached or cached domains
> */
> - domain_remove_dev_info(domain);
> + if (WARN_ON(!list_empty(>devices)))
> + return;
>
warning is good but it doesn't mean the driver shouldn't deal with
that situation to make it safer e.g. blocking DMA from all attached
device...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
On Fri, May 27, 2022 at 02:30:16PM +0800, Lu Baolu wrote: > When the IOMMU domain is about to be freed, it should not be set on any > device. Instead of silently dealing with some bug cases, it's better to > trigger a warning to report and fix any potential bugs at the first time. > > Signed-off-by: Lu Baolu > --- > drivers/iommu/intel/iommu.c | 17 ++--- > 1 file changed, 2 insertions(+), 15 deletions(-) Reviewed-by: Jason Gunthorpe Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 09/12] iommu/vt-d: Check device list of domain in domain free path
When the IOMMU domain is about to be freed, it should not be set on any
device. Instead of silently dealing with some bug cases, it's better to
trigger a warning to report and fix any potential bugs at the first time.
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/iommu.c | 17 ++---
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2e195a639502..6f3119c68cd2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -294,7 +294,6 @@ static LIST_HEAD(dmar_satc_units);
/* bitmap for indexing intel_iommus */
static int g_num_of_iommus;
-static void domain_remove_dev_info(struct dmar_domain *domain);
static void dmar_remove_one_dev_info(struct device *dev);
static void __dmar_remove_one_dev_info(struct device_domain_info *info);
@@ -1835,9 +1834,8 @@ static inline int guestwidth_to_adjustwidth(int gaw)
static void domain_exit(struct dmar_domain *domain)
{
-
- /* Remove associated devices and clear attached or cached domains */
- domain_remove_dev_info(domain);
+ if (WARN_ON(!list_empty(>devices)))
+ return;
if (domain->pgd) {
LIST_HEAD(freelist);
@@ -2328,17 +2326,6 @@ static void domain_context_clear_one(struct
device_domain_info *info, u8 bus, u8
__iommu_flush_dev_iotlb(info, 0, MAX_AGAW_PFN_WIDTH);
}
-static void domain_remove_dev_info(struct dmar_domain *domain)
-{
- struct device_domain_info *info, *tmp;
- unsigned long flags;
-
- spin_lock_irqsave(_domain_lock, flags);
- list_for_each_entry_safe(info, tmp, >devices, link)
- __dmar_remove_one_dev_info(info);
- spin_unlock_irqrestore(_domain_lock, flags);
-}
-
static int domain_setup_first_level(struct intel_iommu *iommu,
struct dmar_domain *domain,
struct device *dev,
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
