[PATCH 0/3] Fix CONT-PTE/PMD size hugetlb issue when unmapping or migrating

[PATCH 0/3] Fix CONT-PTE/PMD size hugetlb issue when unmapping or migrating

[Baolin Wang]

Hi,

Now migrating a hugetlb page or unmapping a poisoned hugetlb page, we'll
use ptep_clear_flush() and set_pte_at() to nuke the page table entry
and remap it, and this is incorrect for CONT-PTE or CONT-PMD size hugetlb
page, which will cause potential data consistent issue. This patch set
will change to use hugetlb related APIs to fix this issue, please find
details in each patch. Thanks.

Baolin Wang (3):
  mm: change huge_ptep_clear_flush() to return the original pte
  mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration
  mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

 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 +--
 mm/rmap.c                          | 58 +++++++++++++++++++++++---------------
 11 files changed, 67 insertions(+), 51 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] mm: change huge_ptep_clear_flush() to return the original pte

[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 <baolin.wang@linux.alibaba.com>
---
 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(-)

diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index 1242f71..616b2ca 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -39,8 +39,8 @@ extern pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 extern void huge_ptep_set_wrprotect(struct mm_struct *mm,
 				    unsigned long addr, pte_t *ptep);
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-extern void huge_ptep_clear_flush(struct vm_area_struct *vma,
-				  unsigned long addr, pte_t *ptep);
+extern pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+				   unsigned long addr, pte_t *ptep);
 #define __HAVE_ARCH_HUGE_PTE_CLEAR
 extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, unsigned long sz);
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cbace1c..ca8e65c 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -486,19 +486,17 @@ void huge_ptep_set_wrprotect(struct mm_struct *mm,
 		set_pte_at(mm, addr, ptep, pfn_pte(pfn, hugeprot));
 }
 
-void huge_ptep_clear_flush(struct vm_area_struct *vma,
-			   unsigned long addr, pte_t *ptep)
+pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+			    unsigned long addr, pte_t *ptep)
 {
 	size_t pgsize;
 	int ncontig;
 
-	if (!pte_cont(READ_ONCE(*ptep))) {
-		ptep_clear_flush(vma, addr, ptep);
-		return;
-	}
+	if (!pte_cont(READ_ONCE(*ptep)))
+		return ptep_clear_flush(vma, addr, ptep);
 
 	ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
-	clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
+	return get_clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
 }
 
 static int __init hugetlbpage_init(void)
diff --git a/arch/ia64/include/asm/hugetlb.h b/arch/ia64/include/asm/hugetlb.h
index 7e46ebd..65d3811 100644
--- a/arch/ia64/include/asm/hugetlb.h
+++ b/arch/ia64/include/asm/hugetlb.h
@@ -23,8 +23,8 @@ static inline int is_hugepage_only_range(struct mm_struct *mm,
 #define is_hugepage_only_range is_hugepage_only_range
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
 }
 
diff --git a/arch/mips/include/asm/hugetlb.h b/arch/mips/include/asm/hugetlb.h
index c214440..fd69c88 100644
--- a/arch/mips/include/asm/hugetlb.h
+++ b/arch/mips/include/asm/hugetlb.h
@@ -43,16 +43,19 @@ static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 }
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
+	pte_t pte;
+
 	/*
 	 * clear the huge pte entry firstly, so that the other smp threads will
 	 * not get old pte entry after finishing flush_tlb_page and before
 	 * setting new huge pte entry
 	 */
-	huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
+	pte = huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
 	flush_tlb_page(vma, addr);
+	return pte;
 }
 
 #define __HAVE_ARCH_HUGE_PTE_NONE
diff --git a/arch/parisc/include/asm/hugetlb.h b/arch/parisc/include/asm/hugetlb.h
index a69cf9e..25bc560 100644
--- a/arch/parisc/include/asm/hugetlb.h
+++ b/arch/parisc/include/asm/hugetlb.h
@@ -28,8 +28,8 @@ static inline int prepare_hugepage_range(struct file *file,
 }
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
 }
 
diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h
index 6a1a1ac..8a5674f 100644
--- a/arch/powerpc/include/asm/hugetlb.h
+++ b/arch/powerpc/include/asm/hugetlb.h
@@ -43,11 +43,14 @@ static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 }
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
-	huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
+	pte_t pte;
+
+	pte = huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
 	flush_hugetlb_page(vma, addr);
+	return pte;
 }
 
 #define __HAVE_ARCH_HUGE_PTEP_SET_ACCESS_FLAGS
diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index 32c3fd6..f22beda 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -50,10 +50,10 @@ static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 		set_pte(ptep, __pte(_SEGMENT_ENTRY_EMPTY));
 }
 
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long address, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long address, pte_t *ptep)
 {
-	huge_ptep_get_and_clear(vma->vm_mm, address, ptep);
+	return huge_ptep_get_and_clear(vma->vm_mm, address, ptep);
 }
 
 static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
diff --git a/arch/sh/include/asm/hugetlb.h b/arch/sh/include/asm/hugetlb.h
index ae4de7b..e727cc9 100644
--- a/arch/sh/include/asm/hugetlb.h
+++ b/arch/sh/include/asm/hugetlb.h
@@ -21,8 +21,8 @@ static inline int prepare_hugepage_range(struct file *file,
 }
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
 }
 
diff --git a/arch/sparc/include/asm/hugetlb.h b/arch/sparc/include/asm/hugetlb.h
index 53838a1..b50aa6f 100644
--- a/arch/sparc/include/asm/hugetlb.h
+++ b/arch/sparc/include/asm/hugetlb.h
@@ -21,8 +21,8 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
 			      pte_t *ptep);
 
 #define __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
-					 unsigned long addr, pte_t *ptep)
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep)
 {
 }
 
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 896f341..a57d667 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -84,10 +84,10 @@ static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 #endif
 
 #ifndef __HAVE_ARCH_HUGE_PTEP_CLEAR_FLUSH
-static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
+static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
 		unsigned long addr, pte_t *ptep)
 {
-	ptep_clear_flush(vma, addr, ptep);
+	return ptep_clear_flush(vma, addr, ptep);
 }
 #endif
 
-- 
1.8.3.1

Re: [PATCH 1/3] mm: change huge_ptep_clear_flush() to return the original pte

[Mike Kravetz]

On 4/29/22 01:14, Baolin Wang wrote:
> 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 <baolin.wang@linux.alibaba.com>
> ---
>  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 <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH 1/3] mm: change huge_ptep_clear_flush() to return the original pte

[Baolin Wang]

On 5/6/2022 7:15 AM, Mike Kravetz wrote:
> On 4/29/22 01:14, Baolin Wang wrote:
>> 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 <baolin.wang@linux.alibaba.com>
>> ---
>>   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

Cause we did not use the return value of huge_ptep_clear_flush() in 
mm/hugetlb.c.

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

Sure. Will add an explicit casting in next version.

> Not absolutely necessary.
> 
> Acked-by: Mike Kravetz <mike.kravetz@oracle.com>

Thanks.

[PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[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 migrating a hugetlb page, we will get the relevant page table
entry by huge_pte_offset() only once to nuke it and remap it with
a migration pte entry. 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 or remap only one pte
or pmd entry for this CONT-PTE/PMD size hugetlb page, which is
not expected for hugetlb migration. The problem is we can still
continue to modify the subpages' data of a hugetlb page during
migrating a hugetlb page, which can cause a serious data consistent
issue, since we did not nuke the page table entry and set a
migration pte for the subpages of a hugetlb page.

To fix this issue, we should change to use huge_ptep_clear_flush()
to nuke a hugetlb page table, and remap it with set_huge_pte_at()
and set_huge_swap_pte_at() when migrating a hugetlb page, which
already considered the CONT-PTE or CONT-PMD size hugetlb.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/rmap.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 6fdd198..7cf2408 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -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 {
 			flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
+			/* Nuke the page table entry. */
+			pteval = ptep_clear_flush(vma, address, pvmw.pte);
 		}
 
-		/* 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);
+				else
+					set_pte_at(mm, address, pvmw.pte, pteval);
 				ret = false;
 				page_vma_mapped_walk_done(&pvmw);
 				break;
@@ -2024,7 +2029,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
 				       !anon_exclusive, subpage);
 			if (anon_exclusive &&
 			    page_try_share_anon_rmap(subpage)) {
-				set_pte_at(mm, address, pvmw.pte, pteval);
+				if (folio_test_hugetlb(folio))
+					set_huge_pte_at(mm, address, pvmw.pte, pteval);
+				else
+					set_pte_at(mm, address, pvmw.pte, pteval);
 				ret = false;
 				page_vma_mapped_walk_done(&pvmw);
 				break;
@@ -2050,7 +2058,11 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
 				swp_pte = pte_swp_mksoft_dirty(swp_pte);
 			if (pte_uffd_wp(pteval))
 				swp_pte = pte_swp_mkuffd_wp(swp_pte);
-			set_pte_at(mm, address, pvmw.pte, swp_pte);
+			if (folio_test_hugetlb(folio))
+				set_huge_swap_pte_at(mm, address, pvmw.pte,
+						     swp_pte, vma_mmu_pagesize(vma));
+			else
+				set_pte_at(mm, address, pvmw.pte, swp_pte);
 			trace_set_migration_pte(address, pte_val(swp_pte),
 						compound_order(&folio->page));
 			/*
-- 
1.8.3.1

Re: [PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[Mike Kravetz]

On 4/29/22 01:14, Baolin Wang wrote:
> 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>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 6fdd198..7cf2408 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -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 {
>  			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.

> -		/* 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.

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

Re: [PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[Baolin Wang]

On 5/6/2022 7:53 AM, Mike Kravetz wrote:
> On 4/29/22 01:14, Baolin Wang wrote:
>> 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>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 6fdd198..7cf2408 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -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 {
>>   			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.

> 
>> -		/* 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.
> 
> 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);

Re: [PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[Mike Kravetz]

On 5/5/22 20:39, Baolin Wang wrote:
> 
> On 5/6/2022 7:53 AM, Mike Kravetz wrote:
>> On 4/29/22 01:14, Baolin Wang wrote:
>>> 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>
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 6fdd198..7cf2408 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -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 {
>>>               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.
> 
>>
>>> -        /* 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.
>>
>> 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;
}

-- 
Mike Kravetz

Re: [PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[Baolin Wang]

On 5/7/2022 1:56 AM, Mike Kravetz wrote:
> On 5/5/22 20:39, Baolin Wang wrote:
>>
>> On 5/6/2022 7:53 AM, Mike Kravetz wrote:
>>> On 4/29/22 01:14, Baolin Wang wrote:
>>>> 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>
>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>> index 6fdd198..7cf2408 100644
>>>> --- a/mm/rmap.c
>>>> +++ b/mm/rmap.c
>>>> @@ -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 {
>>>>                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.
>>
>>>
>>>> -        /* 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.
>>>
>>> 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.

Yes, you are right.

> 
> 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;
> }

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?

+pte_t huge_ptep_get(pte_t *ptep, unsigned long size)
+{
+       int ncontig;
+       pte_t orig_pte = ptep_get(ptep);
+
+       if (!pte_cont(orig_pte))
+               return orig_pte;
+
+       switch (size) {
+       case CONT_PMD_SIZE:
+               ncontig = CONT_PMDS;
+               break;
+       case CONT_PTE_SIZE:
+               ncontig = CONT_PTES;
+               break;
+       default:
+               WARN_ON_ONCE(1);
+               return orig_pte;
+       }
+
+       for (i = 0; i < ncontig; i++, ptep++) {
+               pte_t pte = ptep_get(ptep);
+
+               if (pte_dirty(pte))
+                       orig_pte = pte_mkdirty(orig_pte);
+
+               if (pte_young(pte))
+                       orig_pte = pte_mkyong(orig_pte);
+       }
+
+       return orig_pte;
+}

Re: [PATCH 2/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when migration

[Baolin Wang]

On 5/7/2022 10:33 AM, Baolin Wang wrote:
> 
> 
> On 5/7/2022 1:56 AM, Mike Kravetz wrote:
>> On 5/5/22 20:39, Baolin Wang wrote:
>>>
>>> On 5/6/2022 7:53 AM, Mike Kravetz wrote:
>>>> On 4/29/22 01:14, Baolin Wang wrote:
>>>>> 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>
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 6fdd198..7cf2408 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -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 {
>>>>>                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.
>>>
>>>>
>>>>> -        /* 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.
>>>>
>>>> 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.
> 
> Yes, you are right.
> 
>>
>> 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;
>> }
> 
> 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?

After some investigation, I send out a RFC patch set[1] to address this 
issue. We can talk about this issue in that thread. Thanks.

[1] 
https://lore.kernel.org/all/cover.1651998586.git.baolin.wang@linux.alibaba.com/

[PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[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.

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 <baolin.wang@linux.alibaba.com>
---
 mm/rmap.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 7cf2408..1e168d7 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -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);
 		} else {
 			flush_cache_page(vma, address, pte_pfn(*pvmw.pte));
-		}
-
-		/*
-		 * Nuke the page table entry. When having to clear
-		 * PageAnonExclusive(), we always have to flush.
-		 */
-		if (should_defer_flush(mm, flags) && !anon_exclusive) {
 			/*
-			 * We clear the PTE but do not flush so potentially
-			 * a remote CPU could still be writing to the folio.
-			 * If the entry was previously clean then the
-			 * architecture must guarantee that a clear->dirty
-			 * transition on a cached TLB entry is written through
-			 * and traps if the PTE is unmapped.
+			 * Nuke the page table entry. When having to clear
+			 * PageAnonExclusive(), we always have to flush.
 			 */
-			pteval = ptep_get_and_clear(mm, address, pvmw.pte);
+			if (should_defer_flush(mm, flags) && !anon_exclusive) {
+				/*
+				 * We clear the PTE but do not flush so potentially
+				 * a remote CPU could still be writing to the folio.
+				 * If the entry was previously clean then the
+				 * architecture must guarantee that a clear->dirty
+				 * transition on a cached TLB entry is written through
+				 * and traps if the PTE is unmapped.
+				 */
+				pteval = ptep_get_and_clear(mm, address, pvmw.pte);
 
-			set_tlb_ubc_flush_pending(mm, pte_dirty(pteval));
-		} else {
-			pteval = ptep_clear_flush(vma, address, pvmw.pte);
+				set_tlb_ubc_flush_pending(mm, pte_dirty(pteval));
+			} else {
+				pteval = ptep_clear_flush(vma, address, pvmw.pte);
+			}
 		}
 
 		/*
-- 
1.8.3.1

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Gerald Schaefer]

On Fri, 29 Apr 2022 16:14:43 +0800
Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 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 <baolin.wang@linux.alibaba.com>
> ---
>  mm/rmap.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 7cf2408..1e168d7 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -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().

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Baolin Wang]

On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
> On Fri, 29 Apr 2022 16:14:43 +0800
> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>> 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 <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/rmap.c | 34 +++++++++++++++++-----------------
>>   1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 7cf2408..1e168d7 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -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);
	}

}

> 
> 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().

Thanks for your explanation. As I said as above, I think we've already 
handled the hugetlb with set_huge_swap_pte_at() in try_to_unmap_one().

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Gerald Schaefer]

On Sat, 30 Apr 2022 11:22:33 +0800
Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 
> 
> On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
> > On Fri, 29 Apr 2022 16:14:43 +0800
> > Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> > 
> >> 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 <baolin.wang@linux.alibaba.com>
> >> ---
> >>   mm/rmap.c | 34 +++++++++++++++++-----------------
> >>   1 file changed, 17 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/mm/rmap.c b/mm/rmap.c
> >> index 7cf2408..1e168d7 100644
> >> --- a/mm/rmap.c
> >> +++ b/mm/rmap.c
> >> @@ -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);
> 	}
> 
> }

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?

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Baolin Wang]

On 5/2/2022 10:02 PM, Gerald Schaefer wrote:
> On Sat, 30 Apr 2022 11:22:33 +0800
> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>>
>>
>> On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
>>> On Fri, 29 Apr 2022 16:14:43 +0800
>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>
>>>> 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 <baolin.wang@linux.alibaba.com>
>>>> ---
>>>>    mm/rmap.c | 34 +++++++++++++++++-----------------
>>>>    1 file changed, 17 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>> index 7cf2408..1e168d7 100644
>>>> --- a/mm/rmap.c
>>>> +++ b/mm/rmap.c
>>>> @@ -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);
>> 	}
>>
>> }
> 
> 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);

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Gerald Schaefer]

On Tue, 3 May 2022 10:19:46 +0800
Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 
> 
> On 5/2/2022 10:02 PM, Gerald Schaefer wrote:
> > On Sat, 30 Apr 2022 11:22:33 +0800
> > Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> > 
> >>
> >>
> >> On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
> >>> On Fri, 29 Apr 2022 16:14:43 +0800
> >>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> >>>
> >>>> 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 <baolin.wang@linux.alibaba.com>
> >>>> ---
> >>>>    mm/rmap.c | 34 +++++++++++++++++-----------------
> >>>>    1 file changed, 17 insertions(+), 17 deletions(-)
> >>>>
> >>>> diff --git a/mm/rmap.c b/mm/rmap.c
> >>>> index 7cf2408..1e168d7 100644
> >>>> --- a/mm/rmap.c
> >>>> +++ b/mm/rmap.c
> >>>> @@ -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);
> >> 	}
> >>
> >> }
> > 
> > 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);

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).

> 
> /* 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.

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Baolin Wang]

On 5/3/2022 6:03 PM, Gerald Schaefer wrote:
> On Tue, 3 May 2022 10:19:46 +0800
> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>>
>>
>> On 5/2/2022 10:02 PM, Gerald Schaefer wrote:
>>> On Sat, 30 Apr 2022 11:22:33 +0800
>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>
>>>>
>>>>
>>>> On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
>>>>> On Fri, 29 Apr 2022 16:14:43 +0800
>>>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>>>
>>>>>> 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 <baolin.wang@linux.alibaba.com>
>>>>>> ---
>>>>>>     mm/rmap.c | 34 +++++++++++++++++-----------------
>>>>>>     1 file changed, 17 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>>> index 7cf2408..1e168d7 100644
>>>>>> --- a/mm/rmap.c
>>>>>> +++ b/mm/rmap.c
>>>>>> @@ -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);
>>>> 	}
>>>>
>>>> }
>>>
>>> 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);
> 
> 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).

OK.

> 
> 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).

Right, I think so too. I'll look at the uffd code in detail, seems need 
another patch to fix the hugetlb for uffd. Thanks for your comments.

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Mike Kravetz]

On 5/3/22 03:03, Gerald Schaefer wrote:
> On Tue, 3 May 2022 10:19:46 +0800
> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>>
>>
>> On 5/2/2022 10:02 PM, Gerald Schaefer wrote:
>>> On Sat, 30 Apr 2022 11:22:33 +0800
>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>
>>>>
>>>>
>>>> On 4/30/2022 4:02 AM, Gerald Schaefer wrote:
>>>>> On Fri, 29 Apr 2022 16:14:43 +0800
>>>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>>>
>>>>>> 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 <baolin.wang@linux.alibaba.com>
>>>>>> ---
>>>>>>    mm/rmap.c | 34 +++++++++++++++++-----------------
>>>>>>    1 file changed, 17 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>>> index 7cf2408..1e168d7 100644
>>>>>> --- a/mm/rmap.c
>>>>>> +++ b/mm/rmap.c
>>>>>> @@ -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);
>>>> 	}
>>>>
>>>> }
>>>
>>> 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);
> 
> 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).
> 

Adding Peter Wu mostly for above as he is working uffd_wp.

>>
>> /* 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.
> 


-- 
Mike Kravetz

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Peter Xu]

On Fri, May 06, 2022 at 12:07:13PM -0700, Mike Kravetz wrote:
> On 5/3/22 03:03, Gerald Schaefer wrote:
> > On Tue, 3 May 2022 10:19:46 +0800
> > Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> >> On 5/2/2022 10:02 PM, Gerald Schaefer wrote:

[...]

> >> 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).

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.

Thanks,

-- 
Peter Xu

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Baolin Wang]

On 5/10/2022 12:41 AM, Peter Xu wrote:
> On Fri, May 06, 2022 at 12:07:13PM -0700, Mike Kravetz wrote:
>> On 5/3/22 03:03, Gerald Schaefer wrote:
>>> On Tue, 3 May 2022 10:19:46 +0800
>>> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>> On 5/2/2022 10:02 PM, Gerald Schaefer wrote:
> 
> [...]
> 
>>>> 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).
> 
> 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.

OK. Thanks Peter.

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Mike Kravetz]

On 4/29/22 01:14, Baolin Wang wrote:
> 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,

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.

-- 
Mike Kravetz

> 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 <baolin.wang@linux.alibaba.com>
> ---
>  mm/rmap.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

Re: [PATCH 3/3] mm: rmap: Fix CONT-PTE/PMD size hugetlb issue when unmapping

[Baolin Wang]

On 5/7/2022 2:55 AM, Mike Kravetz wrote:
> On 4/29/22 01:14, Baolin Wang wrote:
>> 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,
> 
> 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.

Yes, will update in next version.

> 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.
Good point. Will do in next version. Thanks.