Re: [PATCH] iommu/vt-d: debugfs: Increment the reference count of page table page

[Liu, Jingqi]

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.
Good point.
It seems the bit used to check must be differentiated between valid and 
stale.
But the PTE may not change whether it is valid or stale.

Thanks,
Jingqi
> > +			put_page(pg);
> > +		}
> >   		path[level] = 0;
> >   	}
> >   }
> > --
> > 2.21.3