Re: [PATCH] iommu/vt-d: debugfs: Increment the reference count of page table page
From: Liu, Jingqi
Date: Thu Aug 03 2023 - 07:21:50 EST
Hi Kevin,
On 7/3/2023 10:48 PM, Liu, Jingqi wrote:
On 7/3/2023 3:00 PM, Tian, Kevin wrote:
From: Liu, Jingqi <jingqi.liu@xxxxxxxxx>
Sent: Sunday, June 25, 2023 10:28 AM
There may be a race with iommu_unmap() interface when traversing a page
table.
When debugfs traverses an IOMMU page table, iommu_unmap() may clear
entries and free the page table pages pointed to by the entries.
So debugfs may read invalid or freed pages.
To avoid this, increment the refcount of a page table page before
traversing the page, and decrement its refcount after traversing it.
I'm not sure how this race can be fully avoided w/o cooperation in the
unmap path...
Thanks.
Indeed, in order to fully avoid this, need to cooperate in the unmap
path. :)
Signed-off-by: Jingqi Liu <Jingqi.liu@xxxxxxxxx>
---
drivers/iommu/intel/debugfs.c | 36 +++++++++++++++++++++++++++++++++-
-
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/intel/debugfs.c
b/drivers/iommu/intel/debugfs.c
index 1f925285104e..d228e1580aec 100644
--- a/drivers/iommu/intel/debugfs.c
+++ b/drivers/iommu/intel/debugfs.c
@@ -333,9 +333,41 @@ static void pgtable_walk_level(struct seq_file *m,
struct dma_pte *pde,
path[level] = pde->val;
if (dma_pte_superpage(pde) || level == 1)
dump_page_info(m, start, path);
- else
- pgtable_walk_level(m,
phys_to_virt(dma_pte_addr(pde)),
+ else {
+ struct page *pg;
+ u64 pte_addr;
+
+ /*
+ * The entry references a Page-Directory Table
+ * or a Page Table.
+ */
+retry:
+ pte_addr = dma_pte_addr(pde);
+ pg = pfn_to_page(pte_addr >> PAGE_SHIFT);
+ if (!get_page_unless_zero(pg))
+ /*
+ * If this page has a refcount of zero,
+ * it has been freed, or will be freed.
+ */
+ continue;
+
+ /* Check if the value of the entry is changed. */
+ if (pde->val != path[level]) {
+ put_page(pg);
+
+ if (!dma_pte_present(pde))
+ /* The entry is invalid. Skip it. */
+ continue;
+
+ /* The entry has been updated. */
+ path[level] = pde->val;
+ goto retry;
+ }
+
+ pgtable_walk_level(m, phys_to_virt(pte_addr),
level - 1, start, path);
What about pde->val getting cleared after phys_to_virt(pte_addr) leading
to all the levels below 'pg' being freed? In that case this code
still walks
the stale 'pg' content which however all point to invalid pages then.
There are 2 cases for the page pointed to by the PTE below 'pg'.
1) the page has been freed.
It will be skipped after the following check:
if (!get_page_unless_zero(pg))
/*
* If this page has a refcount of zero,
* it has been freed, or will be freed.
*/
continue;
Debugfs won't walk further.
2) The page has not been freed.
The content of this page is stale.
Dumping these stale content seems to be acceptable for debugfs.
If all the PTEs below 'pg' can be cleared before being freed in
the unmap path,
the following check can avoid to walk these stale pages.
if (!dma_pte_present(pde))
continue;
It's probably simpler to just check the format of each PTE (e.g. whether
any reserved bit is set) and if abnormal then break the current level of
walk.
Thanks for your suggestion.
If the PTE references a page directory/table, bit 3 is ignored by
hardware according the spec.
In the IOMMU driver, bit 3 is set to 0 by default.
How about setting bit 3 of the corresponding PTE to 1 in the unmap path
to indicate that the page pointed to by the PTE is stale ?
The code modified in the unmap path is as follows:
static void dma_pte_list_pagetables(struct dmar_domain *domain,
int level, struct dma_pte *pte,
struct list_head *freelist)
{
struct page *pg;
pg = pfn_to_page(dma_pte_addr(pte) >> PAGE_SHIFT);
list_add_tail(&pg->lru, freelist);
+
+ pte->val |= BIT_ULL(3);
......
Then during debugfs traversal, check bit 3 of the PTE before calling
pgtable_walk_level().
If this bit is 1, debugfs will stop traversing.
The related code in debugfs is like below:
+retry:
+ pte_addr = dma_pte_addr(pde);
+ pg = pfn_to_page(pte_addr >> PAGE_SHIFT);
+ if (!get_page_unless_zero(pg))
+ /*
+ * If this page has a refcount of zero,
+ * it has been freed, or will be freed.
+ */
+ continue;
+
+ /*
+ * Check if the page that pointed to
+ * by the PTE is stale.
+ */
+ if (pde->val & BIT_ULL(3)) {
+ put_page(pg);
+ continue;
+ }
+
+ /* Check if the value of this entry is changed. */
+ if (pde->val != path[level]) {
+ put_page(pg);
+
+ if (!dma_pte_present(pde))
+ /* This entry is invalid. Skip
it. */
+ continue;
+
+ /* The entry has been updated. */
+ path[level] = pde->val;
+ goto retry;
+ }
+
+ pgtable_walk_level(m, phys_to_virt(pte_addr),
level - 1, start, path);
Thanks,
Jingqi