Re: [PATCH hmm 1/8] mm/hmm: add missing unmaps of the ptep during hmm_vma_handle_pte()

On Wed, Mar 11, 2020 at 06:28:30PM -0700, Ralph Campbell wrote:

...

mm/hmm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c index 72e5a6d9a41756..35f85424176d14 100644 +++ b/mm/hmm.c @@ -325,6 +325,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, } /* Report error for everything else */

• pte_unmap(ptep);
  

  *pfn = range->values[HMM_PFN_ERROR]; return -EFAULT; } else {

@@ -339,10 +340,13 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, if (pte_devmap(pte)) { hmm_vma_walk->pgmap = get_dev_pagemap(pte_pfn(pte), hmm_vma_walk->pgmap);

• if (unlikely(!hmm_vma_walk->pgmap))
  

• if (unlikely(!hmm_vma_walk->pgmap)) {
  

• 	pte_unmap(ptep);
  return -EBUSY;
  

• }
  

  } else if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) && pte_special(pte)) { if (!is_zero_pfn(pte_pfn(pte))) {

• 	pte_unmap(ptep);
  *pfn = range->values[HMM_PFN_SPECIAL];
  return -EFAULT;
  

  }

@@ -437,7 +441,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, &pfns[i]); if (r) {

• 	/* hmm_vma_handle_pte() did unmap pte directory */
  

• 	/* hmm_vma_handle_pte() did pte_unmap() */
  hmm_vma_walk->last = addr;
  return r;
  

  }

I think there is a case where hmm_vma_handle_pte() is called, a fault is requested, pte_unmap() and hmm_vma_walk_hole_() are called, the latter returns zero (the fault was handled OK)

Not quite, hmm_vma_walk_hole_() never returns 0 if called with fault:

return (fault || write_fault) ? -EBUSY : 0;

And all the call sites of hmm_vma_walk_hole_() in hmm_vma_handle_pte() are structured as:

if (fault || write_fault) goto fault; fault: return hmm_vma_walk_hole_(addr, end, fault, write_fault, walk);

So, it never returns 0.

I already made a patch making this less twisty while fixing something else:

https://github.com/jgunthorpe/linux/commit/078e10ca5919f2c263c245784fb5fe63d...

Thanks, Jason