[v2,1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

Message ID 7eea4a30b0f4fdbe14351cf2c6cf537365080d2d.1507078770.git.digetx@gmail.com
State New
Headers show
Series
  • [v2,1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings
Related show

Commit Message

Dmitry Osipenko Oct. 4, 2017, 1:02 a.m.
Due to a bug in IOVA allocator, page mapping could accidentally overwritten.
We can catch this case by checking 'VALID' bit of GART's page entry prior to
mapping of a page. Since that check introduces a noticeable performance
impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG
option.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/Kconfig      |  9 +++++++++
 drivers/iommu/tegra-gart.c | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

Comments

Joerg Roedel Oct. 10, 2017, 9:53 a.m. | #1
On Wed, Oct 04, 2017 at 04:02:31AM +0300, Dmitry Osipenko wrote:
> Due to a bug in IOVA allocator, page mapping could accidentally overwritten.
> We can catch this case by checking 'VALID' bit of GART's page entry prior to
> mapping of a page. Since that check introduces a noticeable performance
> impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG
> option.

Which bug in the IOVA allocator are you talking about?


	Joerg


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Osipenko Oct. 12, 2017, 2:27 p.m. | #2
On 10.10.2017 12:53, Joerg Roedel wrote:
> On Wed, Oct 04, 2017 at 04:02:31AM +0300, Dmitry Osipenko wrote:
>> Due to a bug in IOVA allocator, page mapping could accidentally overwritten.
>> We can catch this case by checking 'VALID' bit of GART's page entry prior to
>> mapping of a page. Since that check introduces a noticeable performance
>> impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG
>> option.
> 
> Which bug in the IOVA allocator are you talking about?
> 

I'm not talking about any specific bug, but in general if allocator re-maps
already mapped region or unmaps the wrong-and-used region. I had those bug-cases
during of development of the 'scattered' graphics allocations for Tegra20.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joerg Roedel Oct. 13, 2017, 8:38 a.m. | #3
On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
> I'm not talking about any specific bug, but in general if allocator re-maps
> already mapped region or unmaps the wrong-and-used region. I had those bug-cases
> during of development of the 'scattered' graphics allocations for Tegra20.

The dma-iommu code does not re-map already mapped regions and it doesn't
unmap wrong regions. If it does it should be reported and fixed.

So if you hit any bug there, please report it so that it can be fixed.


Thanks,

	Joerg

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Osipenko Oct. 13, 2017, 12:52 p.m. | #4
On 13.10.2017 11:38, Joerg Roedel wrote:
> On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
>> I'm not talking about any specific bug, but in general if allocator re-maps
>> already mapped region or unmaps the wrong-and-used region. I had those bug-cases
>> during of development of the 'scattered' graphics allocations for Tegra20.
> 
> The dma-iommu code does not re-map already mapped regions and it doesn't
> unmap wrong regions. If it does it should be reported and fixed.
> 

Certainly iommu_map_sg doesn't perform itself any debug checks that I'm
proposing to add to the GART.

Yet we don't use GART in the mainline, right now you may take a look at the WIP
patches here:

https://github.com/grate-driver/linux/commit/9853371164a7f1b5698caee476e7cffe1b446afa

https://github.com/grate-driver/linux/commit/ea1fca4ac932464e7907a7ada8ea2698cab8db65

> So if you hit any bug there, please report it so that it can be fixed.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..851156a4896d 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -242,6 +242,15 @@  config TEGRA_IOMMU_GART
 	  space through the GART (Graphics Address Relocation Table)
 	  hardware included on Tegra SoCs.
 
+config TEGRA_IOMMU_GART_DEBUG
+	bool "Debug Tegra GART IOMMU"
+	depends on TEGRA_IOMMU_GART
+	help
+	  Properly unmap pages and check whether page is already mapped
+	  during of mapping in expense of performance. This allows to
+	  catch double page remappings, caused by a bug in the IOVA
+	  allocator for example.
+
 config TEGRA_IOMMU_SMMU
 	bool "NVIDIA Tegra SMMU Support"
 	depends on ARCH_TEGRA
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index b62f790ad1ba..bc4cb200fa03 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -271,6 +271,7 @@  static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 	unsigned long pfn;
+	unsigned long pte;
 
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return -EINVAL;
@@ -282,6 +283,14 @@  static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+	if (IS_ENABLED(TEGRA_IOMMU_GART_DEBUG)) {
+		pte = gart_read_pte(gart, iova);
+		if (pte & GART_ENTRY_PHYS_ADDR_VALID) {
+			spin_unlock_irqrestore(&gart->pte_lock, flags);
+			dev_err(gart->dev, "Page entry is used already\n");
+			return -EBUSY;
+		}
+	}
 	gart_set_pte(gart, iova, GART_PTE(pfn));
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
@@ -295,6 +304,10 @@  static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 
+	/* don't unmap page entries to achieve better performance */
+	if (!IS_ENABLED(TEGRA_IOMMU_GART_DEBUG))
+		return 0;
+
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return 0;
 
@@ -302,7 +315,8 @@  static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	gart_set_pte(gart, iova, 0);
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return 0;
+
+	return bytes;
 }
 
 static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,