Skip to contentSkip to conversation

mm/huge_memory: Prevent huge zeropage refcount corruption in PMD move

PATCH In review Chris Down 4 participants 8 messages Feb 26 – Mar 7

Subsystems (3)
lore
Share & tools
lore

Chris Down

author
#0 lore raw

PATCH v2 2/3 mm/huge_memory: Prevent huge zeropage refcount corruption in PMD move

After commit d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special"), moved huge zero PMDs must remain special so vm_normal_page_pmd() continues to treat them as special mappings.

move_pages_huge_pmd() currently reconstructs the destination PMD in the huge zero page branch, which drops PMD state such as pmd_special() on architectures with CONFIG_ARCH_HAS_PTE_SPECIAL. As a result, vm_normal_page_pmd() can treat the moved huge zero PMD as a normal page and corrupt its refcount.

Instead of reconstructing the PMD from the folio, derive the destination entry from src_pmdval after pmdp_huge_clear_flush(), then handle the PMD metadata the same way move_huge_pmd() does for moved entries by marking it soft-dirty and clearing uffd-wp.

Fixes d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special")
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/huge_memory.c +2 -1
index fed57951a7cd..8166b5e871ad 100644
@@ -2794,7 +2794,8 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 		_dst_pmd = pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma);
 	} else {
 		src_pmdval = pmdp_huge_clear_flush(src_vma, src_addr, src_pmd);
-		_dst_pmd = folio_mk_pmd(page_folio(src_page), dst_vma->vm_page_prot);
+		_dst_pmd = move_soft_dirty_pmd(src_pmdval);
+		_dst_pmd = clear_uffd_wp_pmd(_dst_pmd);
 	}
 	set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd);
 
Signature · 1 line
2.51.2
#1 lore raw
Quoted 27 lines · Chris Down

After commit d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special"), moved huge zero PMDs must remain special so vm_normal_page_pmd() continues to treat them as special mappings.

move_pages_huge_pmd() currently reconstructs the destination PMD in the huge zero page branch, which drops PMD state such as pmd_special() on architectures with CONFIG_ARCH_HAS_PTE_SPECIAL. As a result, vm_normal_page_pmd() can treat the moved huge zero PMD as a normal page and corrupt its refcount.

Instead of reconstructing the PMD from the folio, derive the destination entry from src_pmdval after pmdp_huge_clear_flush(), then handle the PMD metadata the same way move_huge_pmd() does for moved entries by marking it soft-dirty and clearing uffd-wp.

Fixes d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special")
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/huge_memory.c +0 -0
index fed57951a7cd..8166b5e871ad 100644

mm/huge_memory.c int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm · line 2798

Quoted 6 lines
mm/huge_memory.c +2 -1
@@ -2794,7 +2794,8 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 		_dst_pmd = pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma);
 	} else {
 		src_pmdval = pmdp_huge_clear_flush(src_vma, src_addr, src_pmd);
-		_dst_pmd = folio_mk_pmd(page_folio(src_page), dst_vma->vm_page_prot);
+		_dst_pmd = move_soft_dirty_pmd(src_pmdval);
+		_dst_pmd = clear_uffd_wp_pmd(_dst_pmd);

Please squash that patch directly in #1.

It doesn't make sense to leave something partially fixed in #1. It's been completely broken from the start. folio_mk_pmd() should never have been used.

Apart from that, the end results LGTM, thanks

Signature · 3 lines
Cheers,

David
#2 lore raw
Quoted 3 lines · David Hildenbrand (Arm)

It doesn't make sense to leave something partially fixed in #1. It's been completely broken from the start. folio_mk_pmd() should never have been used.

... mk_huge_pmd() should never have been used ;-)

#3 lore raw
Quoted 6 lines · Matthew Wilcox
Quoted 3 lines · David Hildenbrand (Arm)

It doesn't make sense to leave something partially fixed in #1. It's been completely broken from the start. folio_mk_pmd() should never have been used.

... mk_huge_pmd() should never have been used ;-)

Heh, yes!

You made me grep for it and find some leftovers:

arch/powerpc/mm/book3s64/pgtable.c: * pmd_mkhuge() and mk_huge_pmd() when we update all the

Anybody fancy sending a cleanup? :)

Signature · 3 lines
Cheers,

David
author
#4 lore raw
In reply to David Hildenbrand (Arm)
Quoted 5 lines · David Hildenbrand (Arm)

Please squash that patch directly in #1.

It doesn't make sense to leave something partially fixed in #1. It's been completely broken from the start. folio_mk_pmd() should never have been used.

The reason was to make sure that in theory the correct trees can be updated based on Fixes: for each. But I agree that it probably doesn't really matter.

Quoted 2 lines

Apart from that, the end results LGTM, thanks

#5 lore raw
In reply to Chris Down
Quoted 27 lines · Chris Down

After commit d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special"), moved huge zero PMDs must remain special so vm_normal_page_pmd() continues to treat them as special mappings.

move_pages_huge_pmd() currently reconstructs the destination PMD in the huge zero page branch, which drops PMD state such as pmd_special() on architectures with CONFIG_ARCH_HAS_PTE_SPECIAL. As a result, vm_normal_page_pmd() can treat the moved huge zero PMD as a normal page and corrupt its refcount.

Instead of reconstructing the PMD from the folio, derive the destination entry from src_pmdval after pmdp_huge_clear_flush(), then handle the PMD metadata the same way move_huge_pmd() does for moved entries by marking it soft-dirty and clearing uffd-wp.

Fixes d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special")
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/huge_memory.c +0 -0
index fed57951a7cd..8166b5e871ad 100644

mm/huge_memory.c int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm · line 2798

Quoted 6 lines
mm/huge_memory.c +2 -1
@@ -2794,7 +2794,8 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 		_dst_pmd = pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma);
 	} else {
 		src_pmdval = pmdp_huge_clear_flush(src_vma, src_addr, src_pmd);
-		_dst_pmd = folio_mk_pmd(page_folio(src_page), dst_vma->vm_page_prot);
+		_dst_pmd = move_soft_dirty_pmd(src_pmdval);
+		_dst_pmd = clear_uffd_wp_pmd(_dst_pmd);

I'm confused as to what's going on here, it seems like the 2/3 is simply updating the 1/3 with a different fixes?

I agree with David that just moving it is probably completely fine, so I think this should be the only actual patch you need, and you can just Fixes:
e3981db444a0 with it? Then make this a v3 series with 2 patches this + the test right (but maybe best not backport the test :)?

Quoted 8 lines
mm/huge_memory.c +0 -1
 	}
 	set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd);
 
--

2.51.2

#6 lore raw

TL;DR - To make life easier, I squashed the two patches and asked Andrew to take it with my R-b,T-b tags attached, hope that's ok with you Chris.

It takes your work, combines commit msgs and the code into one patch with correct attribution to you.

See https://lore.kernel.org/linux-mm/[email protected]/

Thanks, Lorenzo

Quoted 51 lines · Lorenzo Stoakes
Quoted 27 lines · Chris Down

After commit d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special"), moved huge zero PMDs must remain special so vm_normal_page_pmd() continues to treat them as special mappings.

move_pages_huge_pmd() currently reconstructs the destination PMD in the huge zero page branch, which drops PMD state such as pmd_special() on architectures with CONFIG_ARCH_HAS_PTE_SPECIAL. As a result, vm_normal_page_pmd() can treat the moved huge zero PMD as a normal page and corrupt its refcount.

Instead of reconstructing the PMD from the folio, derive the destination entry from src_pmdval after pmdp_huge_clear_flush(), then handle the PMD metadata the same way move_huge_pmd() does for moved entries by marking it soft-dirty and clearing uffd-wp.

Fixes d82d09e48219 ("mm/huge_memory: mark PMD mappings of the huge zero folio special")
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/huge_memory.c +0 -0
index fed57951a7cd..8166b5e871ad 100644

mm/huge_memory.c int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm · line 2798

Quoted 6 lines
mm/huge_memory.c +2 -1
@@ -2794,7 +2794,8 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 		_dst_pmd = pmd_mkwrite(pmd_mkdirty(_dst_pmd), dst_vma);
 	} else {
 		src_pmdval = pmdp_huge_clear_flush(src_vma, src_addr, src_pmd);
-		_dst_pmd = folio_mk_pmd(page_folio(src_page), dst_vma->vm_page_prot);
+		_dst_pmd = move_soft_dirty_pmd(src_pmdval);
+		_dst_pmd = clear_uffd_wp_pmd(_dst_pmd);

I'm confused as to what's going on here, it seems like the 2/3 is simply updating the 1/3 with a different fixes?

I agree with David that just moving it is probably completely fine, so I think this should be the only actual patch you need, and you can just Fixes:
e3981db444a0 with it? Then make this a v3 series with 2 patches this + the test right (but maybe best not backport the test :)?

Quoted 8 lines
mm/huge_memory.c +0 -1
 	}
 	set_pmd_at(mm, dst_addr, dst_pmd, _dst_pmd);
 
--

2.51.2

author
#7 lore raw
Quoted 2 lines · Lorenzo Stoakes

TL;DR - To make life easier, I squashed the two patches and asked Andrew to take it with my R-b,T-b tags attached, hope that's ok with you Chris.

That's fine, thanks again for sorting this out.