mm/smaps: Don't access young/dirty bit if pte unpresent

PATCH Superseded Peter Xu 5 participants 6 messages Aug 3, 2022 – Aug 5, 2022

Subsystems (3)

Memory management Proc filesystem Memory mapping

lore

Share & tools

lore

Peter Xu

author

2022-08-03 22:03 UTC #0 lore raw

PATCH 1/1 mm/smaps: Don't access young/dirty bit if pte unpresent

These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().

Cc Konstantin Khlebnikov <[email protected]>

Fixes b1d4d9e0cbd0 ("proc/smaps: carefully handle migration entries", 2012-05-31)

Signed-off-by Peter Xu <[email protected]>

---
 fs/proc/task_mmu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/proc/task_mmu.c +4 -3

index 9913f3be9fd2..482f91577f8c 100644
@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	struct vm_area_struct *vma = walk->vma;
 	bool locked = !!(vma->vm_flags & VM_LOCKED);
 	struct page *page = NULL;
-	bool migration = false;
+	bool migration = false, young = false, dirty = false;
 
 	if (pte_present(*pte)) {
 		page = vm_normal_page(vma, addr, *pte);
+		young = pte_young(*pte);
+		dirty = pte_dirty(*pte);
 	} else if (is_swap_pte(*pte)) {
 		swp_entry_t swpent = pte_to_swp_entry(*pte);

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	if (!page)
 		return;
 
-	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-		      locked, migration);
+	smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

Signature · 1 line

2.32.0

Vlastimil Babka (SUSE)

reviewer Acked-by Vlastimil Babka on v1 1/1

2022-08-04 09:02 UTC #1 lore raw

Quoted 6 lines · Peter Xu

These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().
Cc Konstantin Khlebnikov <[email protected]>
Fixes b1d4d9e0cbd0 ("proc/smaps: carefully handle migration entries", 2012-05-31)
Signed-off-by Peter Xu <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 32 lines

---
 fs/proc/task_mmu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/proc/task_mmu.c +4 -3

index 9913f3be9fd2..482f91577f8c 100644
@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	struct vm_area_struct *vma = walk->vma;
 	bool locked = !!(vma->vm_flags & VM_LOCKED);
 	struct page *page = NULL;
-	bool migration = false;
+	bool migration = false, young = false, dirty = false;
 
 	if (pte_present(*pte)) {
 		page = vm_normal_page(vma, addr, *pte);
+		young = pte_young(*pte);
+		dirty = pte_dirty(*pte);
 	} else if (is_swap_pte(*pte)) {
 		swp_entry_t swpent = pte_to_swp_entry(*pte);

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	if (!page)
 		return;
 
-	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-		      locked, migration);
+	smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

David Hildenbrand

Reviewed-by on v1 1/1

2022-08-04 11:01 UTC #2 lore raw

In reply to Peter Xu

Quoted 29 lines · Peter Xu

These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().
Cc Konstantin Khlebnikov <[email protected]>
Fixes b1d4d9e0cbd0 ("proc/smaps: carefully handle migration entries", 2012-05-31)
Signed-off-by Peter Xu <[email protected]>
---
 fs/proc/task_mmu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/proc/task_mmu.c +3 -1
index 9913f3be9fd2..482f91577f8c 100644
@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	struct vm_area_struct *vma = walk->vma;
 	bool locked = !!(vma->vm_flags & VM_LOCKED);
 	struct page *page = NULL;
-	bool migration = false;
+	bool migration = false, young = false, dirty = false;
 
 	if (pte_present(*pte)) {
 		page = vm_normal_page(vma, addr, *pte);
+		young = pte_young(*pte);
+		dirty = pte_dirty(*pte);
 	} else if (is_swap_pte(*pte)) {
 		swp_entry_t swpent = pte_to_swp_entry(*pte);
 

fs/proc/task_mmu.c static void smaps_pte_entry(pte_t *pte, unsigned long addr, · line 568

Quoted 9 lines

fs/proc/task_mmu.c +1 -2

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	if (!page)
 		return;
 
-	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-		      locked, migration);
+	smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Yang Shi

Reviewed-by on v1 1/1

2022-08-04 17:44 UTC #3 lore raw

In reply to Peter Xu

Quoted 7 lines · Peter Xu

These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().
Cc Konstantin Khlebnikov <[email protected]>
Fixes b1d4d9e0cbd0 ("proc/smaps: carefully handle migration entries", 2012-05-31)
Signed-off-by Peter Xu <[email protected]>

Reviewed-by Yang Shi <[email protected]>

Quoted 35 lines

---
 fs/proc/task_mmu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/proc/task_mmu.c +4 -4

index 9913f3be9fd2..482f91577f8c 100644
@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
        struct vm_area_struct *vma = walk->vma;
        bool locked = !!(vma->vm_flags & VM_LOCKED);
        struct page *page = NULL;
-       bool migration = false;
+       bool migration = false, young = false, dirty = false;
 
        if (pte_present(*pte)) {
                page = vm_normal_page(vma, addr, *pte);
+               young = pte_young(*pte);
+               dirty = pte_dirty(*pte);
        } else if (is_swap_pte(*pte)) {
                swp_entry_t swpent = pte_to_swp_entry(*pte);

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
        if (!page)
                return;
 
-       smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-                     locked, migration);
+       smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
--

2.32.0

Andrew Morton

maintainer

2022-08-04 22:40 UTC #4 lore raw

In reply to Peter Xu

Quoted 2 lines · On Wed, 3 Aug 2022 18:03:29 -0400 Peter Xu <[email protected]> wrote:

These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().

Please (always) describe the user visible runtime effects of the bug which is being fixed?

Quoted 28 lines

fs/proc/task_mmu.c +4 -3

@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	struct vm_area_struct *vma = walk->vma;
 	bool locked = !!(vma->vm_flags & VM_LOCKED);
 	struct page *page = NULL;
-	bool migration = false;
+	bool migration = false, young = false, dirty = false;
 
 	if (pte_present(*pte)) {
 		page = vm_normal_page(vma, addr, *pte);
+		young = pte_young(*pte);
+		dirty = pte_dirty(*pte);
 	} else if (is_swap_pte(*pte)) {
 		swp_entry_t swpent = pte_to_swp_entry(*pte);

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	if (!page)
 		return;
 
-	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-		      locked, migration);
+	smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

Signature · 1 line

2.32.0

Peter Xu

author

2022-08-05 15:52 UTC #5 lore raw

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Wed, 3 Aug 2022 18:03:29 -0400 Peter Xu <[email protected]> wrote:
These bits should only be valid when the ptes are present. Introducing two booleans for it and set it to false when !pte_present().
Please (always) describe the user visible runtime effects of the bug which is being fixed?

Will do in the future.

For this specific one I'll add some more details:

The bug is found during code reading and no real world issue reported, but logically such an error can cause incorrect readings for either smaps or smaps_rollup output on quite a few fields.

For example, it could cause over-estimate on values like Shared_Dirty, Private_Dirty, Referenced; or it could also cause under-estimate on values like LazyFree, Shared_Clean, Private_Clean.

Meanwhile I think I overlooked the pmd handling which seems to have the same issue but done in another commit.. Will repost soon.

Thanks,

Quoted 31 lines

Quoted 30 lines

fs/proc/task_mmu.c +4 -3

@@ -527,10 +527,12 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	struct vm_area_struct *vma = walk->vma;
 	bool locked = !!(vma->vm_flags & VM_LOCKED);
 	struct page *page = NULL;
-	bool migration = false;
+	bool migration = false, young = false, dirty = false;
 
 	if (pte_present(*pte)) {
 		page = vm_normal_page(vma, addr, *pte);
+		young = pte_young(*pte);
+		dirty = pte_dirty(*pte);
 	} else if (is_swap_pte(*pte)) {
 		swp_entry_t swpent = pte_to_swp_entry(*pte);

@@ -560,8 +562,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
 	if (!page)
 		return;
 
-	smaps_account(mss, page, false, pte_young(*pte), pte_dirty(*pte),
-		      locked, migration);
+	smaps_account(mss, page, false, young, dirty, locked, migration);
 }
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE

Signature · 1 line

2.32.0

Signature · 1 line

Peter Xu