mm/khugepaged: guard is_zero_pfn() calls with pte_present()
Subsystems (2)
Lance Yang
reviewerPATCH 1/1 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.
--- mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
mm/khugepaged.c +8 -5
@@ -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,9 +690,10 @@ 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))) { +if (!pte_none (pteval)) {/* * ptl mostly unnecessary. */
@@ -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 ||
Signature · 1 line
2.49.0
Dev Jain
reviewerQuoted 9 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.
Quoted 6 lines
So, let's fix this potential bug by ensuring all calls to
is_zero_pfn()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
Thanks, I missed this.
mm/khugepaged.c static void release_pte_pages(pte_t *pte, pte_t *_pte, · line 519
Quoted 13 lines
mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)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))
There should be no chance that we end up with a pteval which is not none *and* not present, if you look at the callers of release_pte_pages. So perhaps we should keep this either the same, or, after "if(pte_none(pteval))", do a WARN_ON_ONCE(!pte_present(pteval))?
Quoted 36 lines
mm/khugepaged.c +7
-4continue ; pfn =pte_pfn (pteval);if (is_zero_pfn (pfn))@@ -690,9 +690,10 @@ 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))) { +if (!pte_none (pteval)) {/* * ptl mostly unnecessary. */ @@ -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 ||
Lance Yang
reviewerQuoted 11 lines · Dev Jain
Quoted 9 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.
Quoted 8 lines
Quoted 6 lines
So, let's fix this potential bug by ensuring all calls to
is_zero_pfn()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
Thanks, I missed this.
Me too ...
Quoted 4 lines
Quoted 4 lines
mm/khugepaged.c| 13 ++++++++-----1 file changed, 8 insertions(+), 5 deletions(-)
Quoted 15 lines
mm/khugepaged.c static void release_pte_pages(pte_t *pte, pte_t · line 519
Quoted 10 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))There should be no chance that we end up with a pteval which is not none *and* not present, if you look at the callers of
release_pte_pages. So perhaps we should keep this either the same, or, after "if(pte_none(pteval))", do a WARN_ON_ONCE(!pte_present(pteval))?
Good catch! Yeah, but I'd rather not rely on the callers ...
Wouldn't it just be simpler and safer to always have is_zero_pfn() guarded by pte_present()?
I don't have a strong opinon here, though ;p
Dev, Thanks!
Dev Jain
reviewerQuoted 45 lines · Lance Yang
Quoted 11 lines · Dev Jain
Quoted 9 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.
Quoted 8 lines
Quoted 6 lines
So, let's fix this potential bug by ensuring all calls to
is_zero_pfn()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
Thanks, I missed this.
Me too ...
Quoted 4 lines
Quoted 4 lines
mm/khugepaged.c| 13 ++++++++-----1 file changed, 8 insertions(+), 5 deletions(-)Quoted 15 lines
mm/khugepaged.c static void release_pte_pages(pte_t *pte, pte_t · line 519
Quoted 10 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))There should be no chance that we end up with a pteval which is not none *and* not present, if you look at the callers of
release_pte_pages. So perhaps we should keep this either the same, or, after "if(pte_none(pteval))", do a WARN_ON_ONCE(!pte_present(pteval))?
Quoted 6 lines
Good catch! Yeah, but I'd rather not rely on the callers ...
Wouldn't it just be simpler and safer to always have
is_zero_pfn()guarded bypte_present()?I don't have a strong opinon here, though ;p
Yeah same, I think we can leave it to what you have done.
Quoted 3 lines
Dev, Thanks!
Quoted 32 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>--- mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)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 ; pfn =pte_pfn (pteval);if (is_zero_pfn (pfn))
mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 696
Quoted 9 lines
mm/khugepaged.c +3
-2@@ -690,9 +690,10 @@ 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))) { +if (!pte_none (pteval)) {
Could save a level of indentation by saying
if (pte_none(pteval))
continue;
That would make it crystal clear that we do nothing when pte is none, and we do something when pte is pointing to zero pfn.
Quoted 23 lines
mm/khugepaged.c +4
-2/* * ptl mostly unnecessary. */ @@ -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 ||
Lance Yang
reviewerQuoted 49 lines · Dev Jain
Quoted 32 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
mm/khugepaged.c| 13 ++++++++-----1 file changed, 8 insertions(+), 5 deletions(-)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; pfn = pte_pfn(pteval); if (is_zero_pfn(pfn))mm/khugepaged.c static void · line 697
Quoted 10 lines
mm/khugepaged.c +3
-2@@ -690,9 +690,10 @@ 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))) { + if (!pte_none(pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))
continue;
Quoted 2 lines
That would make it crystal clear that we do nothing when pte is none, and we do something when pte is pointing to zero pfn.
Yes! That does look clearer ;)
I'll pick that up in the next spin, if one is needed.
Quoted 2 lines
Reviewed-by Dev Jain <[email protected]>
Cheers!
Quoted 25 lines
Quoted 25 lines
mm/khugepaged.c +0
-0/** ptl mostly unnecessary. */
mm/khugepaged.c +4
-2@@ -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(structmm_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 ||
Wei Yang
Reviewed-byQuoted 38 lines · Dev Jain
Quoted 36 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>--- mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)mm/khugepaged.c +2
-2@@ -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,9 +690,10 @@ 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) ||
Quoted 9 lines
mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 696
Quoted 4 lines
mm/khugepaged.c +2
-1+ (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))) { +if (!pte_none (pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))
continue;
Vote for this :-)
No other comment.
Signature · 2 lines
Wei Yang Help you, Help me
Quoted 49 lines · Wei Yang
Quoted 45 lines · Dev Jain
Quoted 31 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>--- mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)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 ; pfn =pte_pfn (pteval);if (is_zero_pfn (pfn))mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 696
Quoted 9 lines
mm/khugepaged.c +3
-2@@ -690,9 +690,10 @@ 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))) { +if (!pte_none (pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))Quoted 2 lines
continue;
Quoted 1 line
Vote for this :-)
I suspect there will be a v2, correct?
Signature · 3 lines
Cheers David / dhildenb
Lance Yang
reviewerQuoted 52 lines · David Hildenbrand
Quoted 51 lines · Wei Yang
Quoted 49 lines · Dev Jain
Quoted 32 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
mm/khugepaged.c| 13 ++++++++-----1 file changed, 8 insertions(+), 5 deletions(-)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; pfn = pte_pfn(pteval); if (is_zero_pfn(pfn))mm/khugepaged.c static void · line 697
Quoted 10 lines
mm/khugepaged.c +3
-2@@ -690,9 +690,10 @@ 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))) { + if (!pte_none(pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))
continue;
Quoted 3 lines
Quoted 1 line
Vote for this :-)
I suspect there will be a v2, correct?
I was hoping a v2 wouldn't be necessary for this ;p
Of course, if we'd prefer a v2, I'm happy to send one out.
Cheers!
Quoted 56 lines · Lance Yang
Quoted 53 lines · David Hildenbrand
Quoted 52 lines · Wei Yang
Quoted 50 lines · Dev Jain
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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>--- mm/khugepaged.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)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; pfn = pte_pfn(pteval); if (is_zero_pfn(pfn))mm/khugepaged.c static void · line 697
Quoted 10 lines
mm/khugepaged.c +3
-2@@ -690,9 +690,10 @@ 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))) { + if (!pte_none(pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))
continue;
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Vote for this :-)
I suspect there will be a v2, correct?
I was hoping a v2 wouldn't be necessary for this ;p
Of course, if we'd prefer a v2, I'm happy to send one out.
I lost track of what the result will be, so a v2 would be nice at least for me :)
Signature · 3 lines
Cheers David / dhildenb
Lance Yang
reviewerQuoted 59 lines · David Hildenbrand
Quoted 58 lines · Lance Yang
Quoted 55 lines · David Hildenbrand
Quoted 53 lines · Wei Yang
Quoted 41 lines · Dev Jain
Quoted 39 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()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
mm/khugepaged.c| 13 ++++++++-----1 file changed, 8 insertions(+), 5 deletions(-)mm/khugepaged.c +2
-2@@ -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,9 +690,10 @@ 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) ||Quoted 9 lines
mm/khugepaged.c static void __collapse_huge_page_copy_succeeded(pte_t *pte, · line 696
Quoted 4 lines
mm/khugepaged.c +2
-1+ (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))) { + if (!pte_none(pteval)) {Could save a level of indentation by saying
if (pte_none(pteval))
continue;Vote for this :-)
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I suspect there will be a v2, correct?
I was hoping a v2 wouldn't be necessary for this ;p
Of course, if we'd prefer a v2, I'm happy to send one out.
I lost track of what the result will be, so a v2 would be nice at least for me :)
Sure. V2 on the way ;)
Quoted 1 line · Lance Yang
Sure. V2 on the way ;)
Will wait for that before review then :)
Quoted 6 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 6 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()inkhugepaged.care properly guarded by apte_present()check.
Does it more like to guard pte_pfn() with pte_present()?
Signature · 2 lines
Wei Yang Help you, Help me
Lance Yang
reviewerQuoted 7 lines · Wei Yang
Quoted 6 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 8 lines
Quoted 6 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()inkhugepaged.care properly guarded by apte_present()check.Does it more like to guard
pte_pfn()withpte_present()?
Exactly! My thinking was that by guarding pte_pfn(), we're ultimately protecting is_zero_pfn() from acting on a junk PFN.
So we're on the same page — I just described the end goal ;p
Thanks!
Quoted 9 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.
Quoted 6 lines
So, let's fix this potential bug by ensuring all calls to
is_zero_pfn()inkhugepaged.care properly guarded by apte_present()check.Suggested-by Lorenzo Stoakes <[email protected]>Signed-off-by Lance Yang <[email protected]>---
LGTM.