mm/khugepaged: guard is_zero_pfn() calls with pte_present()

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Suggested-by Lorenzo Stoakes <[email protected]>

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

 mm/khugepaged.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +16 -14

index d635d821f611..648d9335de00 100644
@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
                pte_t pteval = ptep_get(_pte);
                unsigned long pfn;
 
-               if (pte_none(pteval))
+               if (!pte_present(pteval))
                        continue;
                pfn = pte_pfn(pteval);
                if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
             address += nr_ptes * PAGE_SIZE) {
                nr_ptes = 1;
                pteval = ptep_get(_pte);
-               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+               if (pte_none(pteval) ||
+                   (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
                        add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
-                       if (is_zero_pfn(pte_pfn(pteval))) {
-                               /*
-                                * ptl mostly unnecessary.
-                                */
-                               spin_lock(ptl);
-                               ptep_clear(vma->vm_mm, address, _pte);
-                               spin_unlock(ptl);
-                               ksm_might_unmap_zero_page(vma->vm_mm, pteval);
-                       }
+                       if (pte_none(pteval))
+                               continue;
+                       /*
+                        * ptl mostly unnecessary.
+                        */
+                       spin_lock(ptl);
+                       ptep_clear(vma->vm_mm, address, _pte);
+                       spin_unlock(ptl);
+                       ksm_might_unmap_zero_page(vma->vm_mm, pteval);
                } else {
                        struct page *src_page = pte_page(pteval);
 

@@ -794,7 +795,8 @@ static int __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
                unsigned long src_addr = address + i * PAGE_SIZE;
                struct page *src_page;
 
-               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+               if (pte_none(pteval) ||
+                   (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
                        clear_user_highpage(page, src_addr);
                        continue;
                }

@@ -1294,7 +1296,8 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
                                goto out_unmap;
                        }
                }
-               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+               if (pte_none(pteval) ||
+                   (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
                        ++none_or_zero;
                        if (!userfaultfd_armed(vma) &&
                            (!cc->is_khugepaged ||
--

2.49.0

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Suggested-by Lorenzo Stoakes <[email protected]>

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

  mm/khugepaged.c | 29 ++++++++++++++++-------------
  1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +0 -0

index d635d821f611..648d9335de00 100644

Quoted 33 lines · Lance Yang

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Suggested-by Lorenzo Stoakes <[email protected]>

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
  - Collect Reviewed-by from Dev, Wei and Baolin - thanks!
  - Reduce a level of indentation (per Dev)
  - https://lore.kernel.org/linux-mm/20251016033643.10848-1- [email protected]/

  mm/khugepaged.c | 29 ++++++++++++++++-------------

  1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +0 -0

index d635d821f611..648d9335de00 100644

mm/khugepaged.c static void release_pte_pages(pte_t *pte, pte_t · line 520

Quoted 6 lines

mm/khugepaged.c +1 -1

@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t 
 *_pte,
           pte_t pteval = ptep_get(_pte);
           unsigned long pfn;
-        if (pte_none(pteval))
+        if (!pte_present(pteval))
               continue;

Isn't it rather that if we would ever get a !pte_none() && ! pte_present() here, something would be deeply flawed?

I'd much rather spell that out and do here

VM_WARN_ON_ONCE(!pte_present(pteval));

keeping the original check.

mm/khugepaged.c static void · line 695

Quoted 12 lines

mm/khugepaged.c +2 -1

           pfn = pte_pfn(pteval);
           if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void 
 __collapse_huge_page_copy_succeeded(pte_t *pte,
            address += nr_ptes * PAGE_SIZE) {
           nr_ptes = 1;
           pteval = ptep_get(_pte);
-        if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+        if (pte_none(pteval) ||
+            (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {

This now seems to be a common pattern now :)

Should we have a simple helper

static inline void pte_none_or_zero(pte_t pte) {
    if (pte_none(pte))
        return true;
    return pte_present(pte) && is_zero_pfn(pte_pfn(pte)
}

initially maybe local to this file?

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Suggested-by Lorenzo Stoakes <[email protected]>

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

 mm/khugepaged.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +3 -3

index d635d821f611..648d9335de00 100644
@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
 		pte_t pteval = ptep_get(_pte);
 		unsigned long pfn;
 
-		if (pte_none(pteval))
+		if (!pte_present(pteval))
 			continue;
 		pfn = pte_pfn(pteval);
 		if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
 	     address += nr_ptes * PAGE_SIZE) {
 		nr_ptes = 1;
 		pteval = ptep_get(_pte);
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
 			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
-			if (is_zero_pfn(pte_pfn(pteval))) {

mm/khugepaged.c +11 -3

+			/*
+			 * ptl mostly unnecessary.
+			 */
+			spin_lock(ptl);
+			ptep_clear(vma->vm_mm, address, _pte);
+			spin_unlock(ptl);
+			ksm_might_unmap_zero_page(vma->vm_mm, pteval);
 		} else {
 			struct page *src_page = pte_page(pteval);
 

@@ -794,7 +795,8 @@ static int __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
 		unsigned long src_addr = address + i * PAGE_SIZE;
 		struct page *src_page;
 
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
 			clear_user_highpage(page, src_addr);
 			continue;
 		}

@@ -1294,7 +1296,8 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
 				goto out_unmap;
 			}
 		}
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
 			++none_or_zero;
 			if (!userfaultfd_armed(vma) &&
 			    (!cc->is_khugepaged ||
--

2.49.0

Quoted 5 lines · Lance Yang

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

It feels like this somewhat contradicts points I've made on the original series re the is_swap_pte() stuff. Sigh.

I guess that's _such a mess_ it's hard to avoid though.

And I guess it's reasonable that !pte_present() means we can't expect a valid PFN though.

Quoted 5 lines

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

Quoted 4 lines

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Suggested-by Lorenzo Stoakes <[email protected]>

Not sure I really suggested something that strictly contradicts points I made... but I guess I did suggest guarding this stuff more carefully.

Quoted 38 lines

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

  mm/khugepaged.c | 29 ++++++++++++++++-------------
  1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +3 -3

index d635d821f611..648d9335de00 100644
@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
  		pte_t pteval = ptep_get(_pte);
  		unsigned long pfn;
 
-		if (pte_none(pteval))
+		if (!pte_present(pteval))
  			continue;
  		pfn = pte_pfn(pteval);
  		if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
  	     address += nr_ptes * PAGE_SIZE) {
  		nr_ptes = 1;
  		pteval = ptep_get(_pte);
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
-			if (is_zero_pfn(pte_pfn(pteval))) {

mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 697

Quoted 10 lines

mm/khugepaged.c +2 -8

-				/*
-				 * ptl mostly unnecessary.
-				 */
-				spin_lock(ptl);
-				ptep_clear(vma->vm_mm, address, _pte);
-				spin_unlock(ptl);
-				ksm_might_unmap_zero_page(vma->vm_mm, pteval);
-			}
+			if (pte_none(pteval))
+				continue;

Yeah I'm not sure I really love this refactoring.

Can be:

		if (!is_swap_pte(pteval)) {
			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
			if (!is_zero_pfn(pte_pfn(pteval)))
				continue;

			...
		}

Doing pte_pfn() on a pte_none() PTE is fine.

Obviously as theree's a lot of hate for is_swap_pte() you could also do:

		if (pte_none(pteval) || pte_present(pteval)) {
			...
		}

Which literally open-codes !is_swap_pte().

At the same time, this makes very clear that PTE none is OK.

Quoted 32 lines

mm/khugepaged.c +11 -3

+			/*
+			 * ptl mostly unnecessary.
+			 */
+			spin_lock(ptl);
+			ptep_clear(vma->vm_mm, address, _pte);
+			spin_unlock(ptl);
+			ksm_might_unmap_zero_page(vma->vm_mm, pteval);
  		} else {
  			struct page *src_page = pte_page(pteval);
 

@@ -794,7 +795,8 @@ static int __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
  		unsigned long src_addr = address + i * PAGE_SIZE;
  		struct page *src_page;
 
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			clear_user_highpage(page, src_addr);
  			continue;
  		}

@@ -1294,7 +1296,8 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
  				goto out_unmap;
  			}
  		}
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			++none_or_zero;
  			if (!userfaultfd_armed(vma) &&
  			    (!cc->is_khugepaged ||
--

Quoted 2 lines

2.49.0

I mean all of this seems super gross anyway. We're constantly open-coding the same check over and over again.

static inline bool pte_is_none_or_zero(pte_t pteval)

{
	if (is_swap_pte(pteval))
		return false;

	return is_zero_pfn(pte_pfn(pteval));
}

Put somewhere in a relevant header file.

Or again, if there's distaste at is_swap_pte(), and here maybe it's more valid not to use it (given name of function).

static inline bool pte_is_none_or_zero(pte_t pteval)

{
	/* Non-present entries do not have a PFN to check. */
	if (!pte_present(pteval))
		return false;

	if (pte_none(pteval))
		return true;

	return is_zero_pfn(pte_pfn(pteval));
}

I think I'm going to do a series to addres the is_swap_pte() mess actually, as this whole thing is very frustrating.

Quoted 3 lines · Lorenzo Stoakes

Quoted 2 lines · Lance Yang

From: Lance Yang <[email protected]>

Quoted 6 lines

Quoted 3 lines

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

It feels like this somewhat contradicts points I've made on the original series re the is_swap_pte() stuff. Sigh.

My bad. I didn't get your point before ...

And this patch is not intended to touch is_swap_pte() ...

Quoted 6 lines

I guess that's _such a mess_ it's hard to avoid though.

And I guess it's reasonable that !pte_present() means we can't expect a valid PFN though.

Yes, I think we expect a valid PFN must be under pte_present().

Quoted 9 lines

Quoted 9 lines

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Quoted 4 lines

Quoted 1 line

Suggested-by Lorenzo Stoakes <[email protected]>

Not sure I really suggested something that strictly contradicts points I made... but I guess I did suggest guarding this stuff more carefully.

Sorry, I didn't catch you again ... Will drop the Suggested-by tag.

Quoted 66 lines

Quoted 39 lines

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

  mm/khugepaged.c | 29 ++++++++++++++++-------------
  1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +3 -3

index d635d821f611..648d9335de00 100644
@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
  		pte_t pteval = ptep_get(_pte);
  		unsigned long pfn;
 
-		if (pte_none(pteval))
+		if (!pte_present(pteval))
  			continue;
  		pfn = pte_pfn(pteval);
  		if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
  	     address += nr_ptes * PAGE_SIZE) {
  		nr_ptes = 1;
  		pteval = ptep_get(_pte);
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
-			if (is_zero_pfn(pte_pfn(pteval))) {

mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 697

Quoted 10 lines

mm/khugepaged.c +2 -8

-				/*
-				 * ptl mostly unnecessary.
-				 */
-				spin_lock(ptl);
-				ptep_clear(vma->vm_mm, address, _pte);
-				spin_unlock(ptl);
-				ksm_might_unmap_zero_page(vma->vm_mm, pteval);
-			}
+			if (pte_none(pteval))
+				continue;

Yeah I'm not sure I really love this refactoring.

Can be:

		if (!is_swap_pte(pteval)) {
			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
			if (!is_zero_pfn(pte_pfn(pteval)))
				continue;

			...
		}

Doing pte_pfn() on a pte_none() PTE is fine.

Obviously as theree's a lot of hate for is_swap_pte() you could also do:

Quoted 7 lines

		if (pte_none(pteval) || pte_present(pteval)) {
			...
		}

Which literally open-codes !is_swap_pte().

At the same time, this makes very clear that PTE none is OK.

Emm, I'd prefer the new helper pte_none_or_zero() here:

if (pte_none_or_zero(pteval)) {
	add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
	if (pte_none(pteval))
		continue;
	....
}

That looks really clean and simple for me ;)

Quoted 53 lines

Quoted 33 lines

mm/khugepaged.c +11 -3

+			/*
+			 * ptl mostly unnecessary.
+			 */
+			spin_lock(ptl);
+			ptep_clear(vma->vm_mm, address, _pte);
+			spin_unlock(ptl);
+			ksm_might_unmap_zero_page(vma->vm_mm, pteval);
  		} else {
  			struct page *src_page = pte_page(pteval);
 

@@ -794,7 +795,8 @@ static int __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
  		unsigned long src_addr = address + i * PAGE_SIZE;
  		struct page *src_page;
 
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			clear_user_highpage(page, src_addr);
  			continue;
  		}

@@ -1294,7 +1296,8 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
  				goto out_unmap;
  			}
  		}
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
  			++none_or_zero;
  			if (!userfaultfd_armed(vma) &&
  			    (!cc->is_khugepaged ||
--

Quoted 2 lines

2.49.0

I mean all of this seems super gross anyway. We're constantly open-coding the same check over and over again.

static inline bool pte_is_none_or_zero(pte_t pteval)

{
	if (is_swap_pte(pteval))
		return false;

	return is_zero_pfn(pte_pfn(pteval));
}

Put somewhere in a relevant header file.

Or again, if there's distaste at is_swap_pte(), and here maybe it's more valid not to use it (given name of function).

static inline bool pte_is_none_or_zero(pte_t pteval)

Quoted 10 lines

{
	/* Non-present entries do not have a PFN to check. */
	if (!pte_present(pteval))
		return false;

	if (pte_none(pteval))
		return true;

	return is_zero_pfn(pte_pfn(pteval));
}

Yeah, I'll put pte_none_or_zero() in this file first.

static inline bool pte_none_or_zero(pte_t pte)

{
	if (pte_none(pte))
		return true;
	return pte_present(pte) && is_zero_pfn(pte_pfn(pte));
}

Quoted 4 lines

I think I'm going to do a series to addres the is_swap_pte() mess actually, as this whole thing is very frustrating.

Excellent! Looking forward to your series to clean that up ;)

Thanks,
Lance

Quoted 10 lines · Lance Yang

Quoted 7 lines · Lorenzo Stoakes

Quoted 5 lines · Lance Yang

From: Lance Yang <[email protected]>

A non-present entry, like a swap PTE, contains completely different data (swap type and offset). pte_pfn() doesn't know this, so if we feed it a non-present entry, it will spit out a junk PFN.

Quoted 4 lines

Quoted 2 lines

It feels like this somewhat contradicts points I've made on the original series re the is_swap_pte() stuff. Sigh.

My bad. I didn't get your point before ...

Don't worry, this is a problem that existed already and needs addressing, series incoming :)

Quoted 3 lines

And this patch is not intended to touch is_swap_pte() ...

Ack

Quoted 8 lines

Quoted 6 lines

I guess that's _such a mess_ it's hard to avoid though.

And I guess it's reasonable that !pte_present() means we can't expect a valid PFN though.

Yes, I think we expect a valid PFN must be under pte_present().

Yes

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

What if that junk PFN happens to match the zeropage's PFN by sheer chance? While really unlikely, this would be really bad if it did.

So, let's fix this potential bug by ensuring all calls to is_zero_pfn() in khugepaged.c are properly guarded by a pte_present() check.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Suggested-by Lorenzo Stoakes <[email protected]>

Not sure I really suggested something that strictly contradicts points I made... but I guess I did suggest guarding this stuff more carefully.

Sorry, I didn't catch you again ... Will drop the Suggested-by tag.

Nah it's fine sorry, I think in general you are doing what I asked.

I'm going to address the is_swap_pte() stuff separately anyway :) have discussed with David off-list a lot. Think I have a sensible plan...

Quoted 77 lines

Quoted 67 lines

Quoted 40 lines

Reviewed-by Dev Jain <[email protected]>

Reviewed-by Baolin Wang <[email protected]>

Reviewed-by Wei Yang <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
Applies against commit 0f22abd9096e in mm-new.

v1 -> v2:
- Collect Reviewed-by from Dev, Wei and Baolin - thanks!
- Reduce a level of indentation (per Dev)
- https://lore.kernel.org/linux-mm/[email protected]/

   mm/khugepaged.c | 29 ++++++++++++++++-------------
   1 file changed, 16 insertions(+), 13 deletions(-)

mm/khugepaged.c +3 -3

index d635d821f611..648d9335de00 100644
@@ -516,7 +516,7 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
   		pte_t pteval = ptep_get(_pte);
   		unsigned long pfn;
 
-		if (pte_none(pteval))
+		if (!pte_present(pteval))
   			continue;
   		pfn = pte_pfn(pteval);
   		if (is_zero_pfn(pfn))

@@ -690,17 +690,18 @@ static void __collapse_huge_page_copy_succeeded(pte_t *pte,
   	     address += nr_ptes * PAGE_SIZE) {
   		nr_ptes = 1;
   		pteval = ptep_get(_pte);
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
   			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
-			if (is_zero_pfn(pte_pfn(pteval))) {

mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 697

Quoted 10 lines

mm/khugepaged.c +2 -8

-				/*
-				 * ptl mostly unnecessary.
-				 */
-				spin_lock(ptl);
-				ptep_clear(vma->vm_mm, address, _pte);
-				spin_unlock(ptl);
-				ksm_might_unmap_zero_page(vma->vm_mm, pteval);
-			}
+			if (pte_none(pteval))
+				continue;

Yeah I'm not sure I really love this refactoring.

Can be:

		if (!is_swap_pte(pteval)) {
			add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
			if (!is_zero_pfn(pte_pfn(pteval)))
				continue;

			...
		}

Doing pte_pfn() on a pte_none() PTE is fine.

Obviously as theree's a lot of hate for is_swap_pte() you could also do:

Quoted 7 lines

		if (pte_none(pteval) || pte_present(pteval)) {
			...
		}

Which literally open-codes !is_swap_pte().

At the same time, this makes very clear that PTE none is OK.

Emm, I'd prefer the new helper pte_none_or_zero() here:

Quoted 7 lines

if (pte_none_or_zero(pteval)) {
	add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
	if (pte_none(pteval))
		continue;
	....
}

That looks really clean and simple for me ;)

Haha yeah sure that's better :)

Quoted 65 lines

Quoted 54 lines

Quoted 34 lines

mm/khugepaged.c +11 -3

+			/*
+			 * ptl mostly unnecessary.
+			 */
+			spin_lock(ptl);
+			ptep_clear(vma->vm_mm, address, _pte);
+			spin_unlock(ptl);
+			ksm_might_unmap_zero_page(vma->vm_mm, pteval);
   		} else {
   			struct page *src_page = pte_page(pteval);
 

@@ -794,7 +795,8 @@ static int __collapse_huge_page_copy(pte_t *pte, struct folio *folio,
   		unsigned long src_addr = address + i * PAGE_SIZE;
   		struct page *src_page;
 
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
   			clear_user_highpage(page, src_addr);
   			continue;
   		}

@@ -1294,7 +1296,8 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
   				goto out_unmap;
   			}
   		}
-		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
+		if (pte_none(pteval) ||
+		    (pte_present(pteval) && is_zero_pfn(pte_pfn(pteval)))) {
   			++none_or_zero;
   			if (!userfaultfd_armed(vma) &&
   			    (!cc->is_khugepaged ||
--

Quoted 2 lines

2.49.0

I mean all of this seems super gross anyway. We're constantly open-coding the same check over and over again.

static inline bool pte_is_none_or_zero(pte_t pteval)

{
	if (is_swap_pte(pteval))
		return false;

	return is_zero_pfn(pte_pfn(pteval));
}

Put somewhere in a relevant header file.

Or again, if there's distaste at is_swap_pte(), and here maybe it's more valid not to use it (given name of function).

static inline bool pte_is_none_or_zero(pte_t pteval)

Quoted 10 lines

{
	/* Non-present entries do not have a PFN to check. */
	if (!pte_present(pteval))
		return false;

	if (pte_none(pteval))
		return true;

	return is_zero_pfn(pte_pfn(pteval));
}

Quoted 8 lines

Yeah, I'll put pte_none_or_zero() in this file first.

static inline bool pte_none_or_zero(pte_t pte)

{
	if (pte_none(pte))
		return true;
	return pte_present(pte) && is_zero_pfn(pte_pfn(pte));
}

Well I intended this to be in some general header file, but it's not obvious actually where would make sense so feel free to put here as a static (no need for inline).

Quoted 7 lines

Quoted 5 lines

I think I'm going to do a series to addres the is_swap_pte() mess actually, as this whole thing is very frustrating.

Excellent! Looking forward to your series to clean that up ;)

Already started on it :)