Fix CONT-PTE/PMD size hugetlb issue when unmapping or migrating

It is incorrect to use ptep_clear_flush() to nuke a hugetlb page table when unmapping or migrating a hugetlb page, and will change to use huge_ptep_clear_flush() instead in the following patches.

So this is a preparation patch, which changes the huge_ptep_clear_flush() to return the original pte to help to nuke a hugetlb page table.

Signed-off-by Baolin Wang <[email protected]>

---
 arch/arm64/include/asm/hugetlb.h   |  4 ++--
 arch/arm64/mm/hugetlbpage.c        | 12 +++++-------
 arch/ia64/include/asm/hugetlb.h    |  4 ++--
 arch/mips/include/asm/hugetlb.h    |  9 ++++++---
 arch/parisc/include/asm/hugetlb.h  |  4 ++--
 arch/powerpc/include/asm/hugetlb.h |  9 ++++++---
 arch/s390/include/asm/hugetlb.h    |  6 +++---
 arch/sh/include/asm/hugetlb.h      |  4 ++--
 arch/sparc/include/asm/hugetlb.h   |  4 ++--
 include/asm-generic/hugetlb.h      |  4 ++--
 10 files changed, 32 insertions(+), 28 deletions(-)

Quoted 8 lines · Baolin Wang

It is incorrect to use ptep_clear_flush() to nuke a hugetlb page table when unmapping or migrating a hugetlb page, and will change to use huge_ptep_clear_flush() instead in the following patches.

So this is a preparation patch, which changes the huge_ptep_clear_flush() to return the original pte to help to nuke a hugetlb page table.

Signed-off-by Baolin Wang <[email protected]>

Quoted 12 lines

---
  arch/arm64/include/asm/hugetlb.h   |  4 ++--
  arch/arm64/mm/hugetlbpage.c        | 12 +++++-------
  arch/ia64/include/asm/hugetlb.h    |  4 ++--
  arch/mips/include/asm/hugetlb.h    |  9 ++++++---
  arch/parisc/include/asm/hugetlb.h  |  4 ++--
  arch/powerpc/include/asm/hugetlb.h |  9 ++++++---
  arch/s390/include/asm/hugetlb.h    |  6 +++---
  arch/sh/include/asm/hugetlb.h      |  4 ++--
  arch/sparc/include/asm/hugetlb.h   |  4 ++--
  include/asm-generic/hugetlb.h      |  4 ++--
  10 files changed, 32 insertions(+), 28 deletions(-)

The above changes look straight forward.
Happy that you Cc'ed impacted arch maintainers so they can at least have a look.

The only user of huge_ptep_clear_flush() today is hugetlb_cow/wp() in mm/hugetlb.c. Any reason why you did not change that code? At least

cast the return of huge_ptep_clear_flush() to void with a comment?

Not absolutely necessary.

Acked-by Mike Kravetz <[email protected]>

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

mm/rmap.c +0 -0

index 6fdd198..7cf2408 100644
@@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
 					break;
 				}

Quoted 4 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

<snip>

Quoted 12 lines

mm/rmap.c +3 -0

index 6fdd198..7cf2408 100644
@@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
  					break;
  				}
  			}
+
+			/* Nuke the hugetlb page table entry */
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);
  		} else {

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 1935

Quoted 5 lines

mm/rmap.c +2 -0

  			flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
+			/* Nuke the page table entry. */
+			pteval = ptep_clear_flush(vma, address, pvmw.pte);
  		}
 

On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set if ANY of the PTE/PMDs had dirty or young set.

Quoted 8 lines

mm/rmap.c +0 -3

-		/* Nuke the page table entry. */
-		pteval = ptep_clear_flush(vma, address, pvmw.pte);
-
  		/* Set the dirty flag on the folio now the pte is gone. */
  		if (pte_dirty(pteval))
  			folio_mark_dirty(folio);

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 2021

Quoted 6 lines

mm/rmap.c +2 -1

@@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
  			pte_t swp_pte;
 
  			if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-				set_pte_at(mm, address, pvmw.pte, pteval);
+				if (folio_test_hugetlb(folio))
+					set_huge_pte_at(mm, address, pvmw.pte, pteval);

And, we will use that pteval for ALL the PTE/PMDs here. So, we would set the dirty or young bit in ALL PTE/PMDs.

Could that cause any issues? May be more of a question for the arm64 people.

Quoted 18 lines · Mike Kravetz

Quoted 4 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

<snip>

Quoted 12 lines

mm/rmap.c +3 -0

index 6fdd198..7cf2408 100644
@@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
                       break;
                   }
               }
+
+            /* Nuke the hugetlb page table entry */
+            pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);
           } else {

Quoted 8 lines

Quoted 5 lines

mm/rmap.c +2 -0

               flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
+            /* Nuke the page table entry. */
+            pteval = ptep_clear_flush(vma, address, pvmw.pte);
           }

On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set if ANY of the PTE/PMDs had dirty or young set.

Right.

Quoted 17 lines

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 2020

Quoted 13 lines

-        /* Nuke the page table entry. */
-        pteval = ptep_clear_flush(vma, address, pvmw.pte); -
          /* Set the dirty flag on the folio now the pte is gone. */ if (pte_dirty(pteval))
              folio_mark_dirty(folio);

mm/rmap.c +2 -1

@@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
               pte_t swp_pte;
                 if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-                set_pte_at(mm, address, pvmw.pte, pteval);
+                if (folio_test_hugetlb(folio))
+                    set_huge_pte_at(mm, address, pvmw.pte, pteval);

And, we will use that pteval for ALL the PTE/PMDs here.  So, we would set the dirty or young bit in ALL PTE/PMDs.

Quoted 1 line

Could that cause any issues?  May be more of a question for the arm64 people.

I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c:

pte = huge_ptep_get_and_clear(mm, address, ptep);
tlb_remove_huge_tlb_entry(h, tlb, ptep, address);
if (huge_pte_dirty(pte))
    set_page_dirty(page);

Quoted 48 lines · Baolin Wang

Quoted 18 lines · Mike Kravetz

Quoted 4 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

<snip>

Quoted 12 lines

mm/rmap.c +3 -0

index 6fdd198..7cf2408 100644
@@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
                       break;
                   }
               }
+
+            /* Nuke the hugetlb page table entry */
+            pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);
           } else {

Quoted 8 lines

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 1935

Quoted 5 lines

mm/rmap.c +2 -0

               flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
+            /* Nuke the page table entry. */
+            pteval = ptep_clear_flush(vma, address, pvmw.pte);
           }
 

On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set if ANY of the PTE/PMDs had dirty or young set.

Right.

Quoted 17 lines

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 2020

Quoted 13 lines

mm/rmap.c +2 -4

-        /* Nuke the page table entry. */
-        pteval = ptep_clear_flush(vma, address, pvmw.pte);
-
           /* Set the dirty flag on the folio now the pte is gone. */
           if (pte_dirty(pteval))
               folio_mark_dirty(folio);

@@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
               pte_t swp_pte;
                 if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-                set_pte_at(mm, address, pvmw.pte, pteval);
+                if (folio_test_hugetlb(folio))
+                    set_huge_pte_at(mm, address, pvmw.pte, pteval);

And, we will use that pteval for ALL the PTE/PMDs here.  So, we would set the dirty or young bit in ALL PTE/PMDs.

Quoted 9 lines

Quoted 1 line

Could that cause any issues?  May be more of a question for the arm64 people.

I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c:

pte = huge_ptep_get_and_clear(mm, address, ptep);
tlb_remove_huge_tlb_entry(h, tlb, ptep, address);
if (huge_pte_dirty(pte))
    set_page_dirty(page);

Agree that it 'should not' cause issues. It just seems inconsistent. This is not a problem specifically with your patch, just the handling of CONT-PTE/PMD entries.

There does not appear to be an arm64 specific version of huge_ptep_get() that takes CONT-PTE/PMD into account. So, huge_ptep_get() would only return the one specific value. It would not take into account the dirty or young bits of CONT-PTE/PMDs like your new version of huge_ptep_get_and_clear. Is that correct? Or, am I missing something.

If I am correct, then code like the following may not work:

static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask,
unsigned long addr, unsigned long end, struct mm_walk *walk)

{
         pte_t huge_pte = huge_ptep_get(pte);
         struct numa_maps *md;
         struct page *page;

         if (!pte_present(huge_pte))
                 return 0;

         page = pte_page(huge_pte);

         md = walk->private;
         gather_stats(page, md, pte_dirty(huge_pte), 1);
         return 0;
}

Quoted 64 lines · Mike Kravetz

Quoted 49 lines · Baolin Wang

Quoted 18 lines · Mike Kravetz

Quoted 4 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

<snip>

Quoted 12 lines

mm/rmap.c +3 -0

index 6fdd198..7cf2408 100644
@@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio 
 *folio, struct vm_area_struct *vma,
                        break;
                    }
                }
+
+            /* Nuke the hugetlb page table entry */
+            pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Quoted 8 lines

mm/rmap.c static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, · line 1934

Quoted 5 lines

mm/rmap.c +2 -0

            } else {
                flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
+            /* Nuke the page table entry. */
+            pteval = ptep_clear_flush(vma, address, pvmw.pte);
            }

On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set if ANY of the PTE/PMDs had dirty or young set.

Right.

Quoted 18 lines

Quoted 8 lines

mm/rmap.c +0 -3

-        /* Nuke the page table entry. */
-        pteval = ptep_clear_flush(vma, address, pvmw.pte);
-
            /* Set the dirty flag on the folio now the pte is gone. */
            if (pte_dirty(pteval))
                folio_mark_dirty(folio);

mm/rmap.c static bool try_to_migrate_one(struct folio · line 2021

Quoted 6 lines

mm/rmap.c +2 -1

@@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio 
 *folio, struct vm_area_struct *vma,
                pte_t swp_pte;
                  if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-                set_pte_at(mm, address, pvmw.pte, pteval);
+                if (folio_test_hugetlb(folio))
+                    set_huge_pte_at(mm, address, pvmw.pte, pteval);

And, we will use that pteval for ALL the PTE/PMDs here.  So, we would set the dirty or young bit in ALL PTE/PMDs.

Quoted 9 lines

Quoted 1 line

Could that cause any issues?  May be more of a question for the arm64 people.

I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c:

pte = huge_ptep_get_and_clear(mm, address, ptep);
tlb_remove_huge_tlb_entry(h, tlb, ptep, address);
if (huge_pte_dirty(pte))
     set_page_dirty(page);

Agree that it 'should not' cause issues.  It just seems inconsistent. This is not a problem specifically with your patch, just the handling of CONT-PTE/PMD entries.

Quoted 5 lines

There does not appear to be an arm64 specific version of huge_ptep_get() that takes CONT-PTE/PMD into account.  So, huge_ptep_get() would only return the one specific value.  It would not take into account the dirty or young bits of CONT-PTE/PMDs like your new version of huge_ptep_get_and_clear.  Is that correct?  Or, am I missing something.

Yes, you are right.

Quoted 13 lines

If I am correct, then code like the following may not work:

static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask, unsigned long addr, unsigned long end, struct mm_walk *walk) {
         pte_t huge_pte = huge_ptep_get(pte);
         struct numa_maps *md;
         struct page *page;

         if (!pte_present(huge_pte))
                 return 0;

Quoted 6 lines

         page = pte_page(huge_pte);

         md = walk->private;
         gather_stats(page, md, pte_dirty(huge_pte), 1); return 0;
}

Right, this is inconsistent with current huge_ptep_get() interface like you said. So I think we can define an ARCH-specific huge_ptep_get() interface for arm64, and some sample code like below. How do you think?

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/rmap.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

mm/rmap.c +0 -0

index 7cf2408..1e168d7 100644

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/rmap.c | 34 +++++++++++++++++-----------------
  1 file changed, 17 insertions(+), 17 deletions(-)

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
  					break;
  				}
  			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

Please note that huge_ptep_get functions do not return valid PTEs on s390, and such PTEs must never be set directly with set_pte_at(), but only with set_huge_pte_at().

Background is that, for hugetlb pages, we are of course not really dealing with PTEs at this level, but rather PMDs or PUDs, depending on hugetlb size. On s390, the layout is quite different for PTEs and PMDs / PUDs, and unfortunately the hugetlb code is not properly reflecting this by using PMD or PUD types, like the THP code does.

So, as work-around, on s390, the huge_ptep_xxx functions will return only fake PTEs, which must be converted again to a proper PMD or PUD, before writing them to the page table, which is what happens in set_huge_pte_at(), but not in set_pte_at().

Quoted 36 lines · Gerald Schaefer

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
  mm/rmap.c | 34 +++++++++++++++++-----------------
  1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
  					break;
  				}
  			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

IIUC, As I said in the commit message, we will only unmap a poisoned hugetlb page by try_to_unmap(), and the poisoned hugetlb page will be remapped with a poisoned entry by set_huge_swap_pte_at() in try_to_unmap_one(). So I think no need change to use set_huge_pte_at() instead of set_pte_at() for other cases, since the hugetlb page will not hit other cases.

if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
	if (folio_test_hugetlb(folio)) {
		hugetlb_count_sub(folio_nr_pages(folio), mm);
		set_huge_swap_pte_at(mm, address, pvmw.pte, pteval,
				     vma_mmu_pagesize(vma));
	} else {
		dec_mm_counter(mm, mm_counter(&folio->page));
		set_pte_at(mm, address, pvmw.pte, pteval);
	}

}

Quoted 73 lines · On Sat, 30 Apr 2022 11:22:33 +0800 Baolin Wang <[email protected]>…

Quoted 36 lines · Gerald Schaefer

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
   mm/rmap.c | 34 +++++++++++++++++-----------------
   1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
   					break;
   				}
   			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

IIUC, As I said in the commit message, we will only unmap a poisoned hugetlb page by try_to_unmap(), and the poisoned hugetlb page will be remapped with a poisoned entry by set_huge_swap_pte_at() in try_to_unmap_one(). So I think no need change to use set_huge_pte_at() instead of set_pte_at() for other cases, since the hugetlb page will not hit other cases.

if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
	if (folio_test_hugetlb(folio)) {
		hugetlb_count_sub(folio_nr_pages(folio), mm);
		set_huge_swap_pte_at(mm, address, pvmw.pte, pteval,
				     vma_mmu_pagesize(vma));
	} else {
		dec_mm_counter(mm, mm_counter(&folio->page));
		set_pte_at(mm, address, pvmw.pte, pteval);
	}

Quoted 1 line

}

OK, but wouldn't the pteval be overwritten here with pteval = swp_entry_to_pte(make_hwpoison_entry(subpage))? IOW, what sense does it make to save the returned pteval from huge_ptep_clear_flush(), when it is never being used anywhere?

Quoted 76 lines · Gerald Schaefer

Quoted 73 lines · On Sat, 30 Apr 2022 11:22:33 +0800 Baolin Wang <[email protected]>…

Quoted 36 lines · Gerald Schaefer

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
   mm/rmap.c | 34 +++++++++++++++++-----------------
   1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
   					break;
   				}
   			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

IIUC, As I said in the commit message, we will only unmap a poisoned hugetlb page by try_to_unmap(), and the poisoned hugetlb page will be remapped with a poisoned entry by set_huge_swap_pte_at() in try_to_unmap_one(). So I think no need change to use set_huge_pte_at() instead of set_pte_at() for other cases, since the hugetlb page will not hit other cases.

if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
	if (folio_test_hugetlb(folio)) {
		hugetlb_count_sub(folio_nr_pages(folio), mm);
		set_huge_swap_pte_at(mm, address, pvmw.pte, pteval,
				     vma_mmu_pagesize(vma));
	} else {
		dec_mm_counter(mm, mm_counter(&folio->page));
		set_pte_at(mm, address, pvmw.pte, pteval);
	}

Quoted 6 lines

Quoted 1 line

}

OK, but wouldn't the pteval be overwritten here with pteval = swp_entry_to_pte(make_hwpoison_entry(subpage))? IOW, what sense does it make to save the returned pteval from huge_ptep_clear_flush(), when it is never being used anywhere?

Please see previous code, we'll use the original pte value to check if it is uffd-wp armed, and if need to mark it dirty though the hugetlbfs is set noop_dirty_folio().

pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);

/* Set the dirty flag on the folio now the pte is gone. */ if (pte_dirty(pteval))

	folio_mark_dirty(folio);

Quoted 90 lines · On Tue, 3 May 2022 10:19:46 +0800 Baolin Wang <[email protected]>…

Quoted 76 lines · Gerald Schaefer

Quoted 73 lines · On Sat, 30 Apr 2022 11:22:33 +0800 Baolin Wang <[email protected]>…

Quoted 36 lines · Gerald Schaefer

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
    mm/rmap.c | 34 +++++++++++++++++-----------------
    1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
    					break;
    				}
    			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

IIUC, As I said in the commit message, we will only unmap a poisoned hugetlb page by try_to_unmap(), and the poisoned hugetlb page will be remapped with a poisoned entry by set_huge_swap_pte_at() in try_to_unmap_one(). So I think no need change to use set_huge_pte_at() instead of set_pte_at() for other cases, since the hugetlb page will not hit other cases.

if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
	if (folio_test_hugetlb(folio)) {
		hugetlb_count_sub(folio_nr_pages(folio), mm);
		set_huge_swap_pte_at(mm, address, pvmw.pte, pteval,
				     vma_mmu_pagesize(vma));
	} else {
		dec_mm_counter(mm, mm_counter(&folio->page));
		set_pte_at(mm, address, pvmw.pte, pteval);
	}

Quoted 6 lines

Quoted 1 line

}

OK, but wouldn't the pteval be overwritten here with pteval = swp_entry_to_pte(make_hwpoison_entry(subpage))? IOW, what sense does it make to save the returned pteval from huge_ptep_clear_flush(), when it is never being used anywhere?

Please see previous code, we'll use the original pte value to check if it is uffd-wp armed, and if need to mark it dirty though the hugetlbfs is set noop_dirty_folio().

Quoted 1 line

pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);

Uh, ok, that wouldn't work on s390, but we also don't have CONFIG_PTE_MARKER_UFFD_WP / HAVE_ARCH_USERFAULTFD_WP set, so I guess we will be fine (for now).

Still, I find it a bit unsettling that pte_install_uffd_wp_if_needed() would work on a potential hugetlb *pte, directly de-referencing it instead of using huge_ptep_get().

The !pte_none(*pte) check at the beginning would be broken in the hugetlb case for s390 (not sure about other archs, but I think s390 might be the only exception strictly requiring huge_ptep_get() for de-referencing hugetlb *pte pointers).

Quoted 86 lines · On Tue, 3 May 2022 10:19:46 +0800 Baolin Wang <[email protected]>…

Quoted 76 lines · Gerald Schaefer

Quoted 73 lines · On Sat, 30 Apr 2022 11:22:33 +0800 Baolin Wang <[email protected]>…

Quoted 36 lines · Gerald Schaefer

Quoted 33 lines · On Fri, 29 Apr 2022 16:14:43 +0800 Baolin Wang <[email protected]>…

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page, which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
   mm/rmap.c | 34 +++++++++++++++++-----------------
   1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1567

Quoted 9 lines

mm/rmap.c +1 -0

index 7cf2408..1e168d7 100644
@@ -1564,28 +1564,28 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
   					break;
   				}
   			}
+			pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);

Unlike in your patch 2/3, I do not see that this (huge) pteval would later be used again with set_huge_pte_at() instead of set_pte_at(). Not sure if this (huge) pteval could end up at a set_pte_at() later, but if yes, then this would be broken on s390, and you'd need to use set_huge_pte_at() instead of set_pte_at() like in your patch 2/3.

IIUC, As I said in the commit message, we will only unmap a poisoned hugetlb page by try_to_unmap(), and the poisoned hugetlb page will be remapped with a poisoned entry by set_huge_swap_pte_at() in try_to_unmap_one(). So I think no need change to use set_huge_pte_at() instead of set_pte_at() for other cases, since the hugetlb page will not hit other cases.

if (PageHWPoison(subpage) && !(flags & TTU_IGNORE_HWPOISON)) {
	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
	if (folio_test_hugetlb(folio)) {
		hugetlb_count_sub(folio_nr_pages(folio), mm);
		set_huge_swap_pte_at(mm, address, pvmw.pte, pteval,
				     vma_mmu_pagesize(vma));
	} else {
		dec_mm_counter(mm, mm_counter(&folio->page));
		set_pte_at(mm, address, pvmw.pte, pteval);
	}

Quoted 6 lines

Quoted 1 line

}

OK, but wouldn't the pteval be overwritten here with pteval = swp_entry_to_pte(make_hwpoison_entry(subpage))? IOW, what sense does it make to save the returned pteval from huge_ptep_clear_flush(), when it is never being used anywhere?

Quoted 5 lines

Please see previous code, we'll use the original pte value to check if it is uffd-wp armed, and if need to mark it dirty though the hugetlbfs is set noop_dirty_folio().

pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);

Uh, ok, that wouldn't work on s390, but we also don't have CONFIG_PTE_MARKER_UFFD_WP / HAVE_ARCH_USERFAULTFD_WP set, so I guess we will be fine (for now).

Still, I find it a bit unsettling that pte_install_uffd_wp_if_needed() would work on a potential hugetlb *pte, directly de-referencing it instead of using huge_ptep_get().

The !pte_none(*pte) check at the beginning would be broken in the hugetlb case for s390 (not sure about other archs, but I think s390 might be the only exception strictly requiring huge_ptep_get() for de-referencing hugetlb *pte pointers).

Quoted 4 lines

/* Set the dirty flag on the folio now the pte is gone. */ if (pte_dirty(pteval))

	folio_mark_dirty(folio);

Ok, that should work fine, huge_ptep_clear_flush() will return a pteval properly de-referenced and converted with huge_ptep_get(), and that would contain the hugetlb pmd/pud dirty information.

Quoted 3 lines · Gerald Schaefer

Quoted 1 line · On Tue, 3 May 2022 10:19:46 +0800 Baolin Wang <[email protected]>…

On 5/2/2022 10:02 PM, Gerald Schaefer wrote:

Quoted 10 lines

Quoted 5 lines

Please see previous code, we'll use the original pte value to check if it is uffd-wp armed, and if need to mark it dirty though the hugetlbfs is set noop_dirty_folio().

pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);

Uh, ok, that wouldn't work on s390, but we also don't have CONFIG_PTE_MARKER_UFFD_WP / HAVE_ARCH_USERFAULTFD_WP set, so I guess we will be fine (for now).

Quoted 8 lines

Still, I find it a bit unsettling that pte_install_uffd_wp_if_needed() would work on a potential hugetlb *pte, directly de-referencing it instead of using huge_ptep_get().

The !pte_none(*pte) check at the beginning would be broken in the hugetlb case for s390 (not sure about other archs, but I think s390 might be the only exception strictly requiring huge_ptep_get() for de-referencing hugetlb *pte pointers).

Quoted 4 lines · Mike Kravetz

Quoted 3 lines · Gerald Schaefer

Quoted 1 line · On Tue, 3 May 2022 10:19:46 +0800 Baolin Wang <[email protected]>…

On 5/2/2022 10:02 PM, Gerald Schaefer wrote:

[...]

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

Please see previous code, we'll use the original pte value to check if it is uffd-wp armed, and if need to mark it dirty though the hugetlbfs is set noop_dirty_folio().

pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);

Uh, ok, that wouldn't work on s390, but we also don't have CONFIG_PTE_MARKER_UFFD_WP / HAVE_ARCH_USERFAULTFD_WP set, so I guess we will be fine (for now).

Quoted 8 lines

Quoted 8 lines

Still, I find it a bit unsettling that pte_install_uffd_wp_if_needed() would work on a potential hugetlb *pte, directly de-referencing it instead of using huge_ptep_get().

The !pte_none(*pte) check at the beginning would be broken in the hugetlb case for s390 (not sure about other archs, but I think s390 might be the only exception strictly requiring huge_ptep_get() for de-referencing hugetlb *pte pointers).

We could have used is_vm_hugetlb_page(vma) within the helper so as to properly use either generic pte or hugetlb version of pte fetching. We may want to conditionally do set_[huge_]pte_at() too at the end.

I could prepare a patch for that even if it's not really anything urgently needed. I assume that won't need to block this patchset since we need the pteval for pte_dirty() check anyway and uffd-wp definitely needs it too.

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

And now we only use try_to_unmap() to unmap a poisoned hugetlb page,

which means now we will unmap only one pte entry for a CONT-PTE or CONT-PMD size poisoned hugetlb page, and we can still access other subpages of a CONT-PTE or CONT-PMD size poisoned hugetlb page, which will cause serious issues possibly.

So we should change to use huge_ptep_clear_flush() to nuke the hugetlb page table to fix this issue, which already considered CONT-PTE and CONT-PMD size hugetlb.

Note we've already used set_huge_swap_pte_at() to set a poisoned swap entry for a poisoned hugetlb page.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/rmap.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Quoted 15 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb:
2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified.

When unmapping a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table.

However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes, so we will nuke only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page.

Quoted 1 line

And now we only use try_to_unmap() to unmap a poisoned hugetlb page,

Since try_to_unmap can be called for non-hugetlb pages, perhaps the following is more accurate?

try_to_unmap is only passed a hugetlb page in the case where the hugetlb page is poisoned.

It does concern me that this assumption is built into the code as pointed out in your discussion with Gerald. Should we perhaps add a VM_BUG_ON() to make sure the passed huge page is poisoned? This would be in the same 'if block' where we call adjust_range_if_pmd_sharing_possible.