mm/huge_memory: Prevent huge zeropage refcount corruption in PMD move
Subsystems (3)
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.
--- mm/huge_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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); }set_pmd_at (mm, dst_addr, dst_pmd, _dst_pmd);
Signature · 1 line
2.51.2
David Hildenbrand (Arm)
maintainerQuoted 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 sovm_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 aspmd_special()on architectures withCONFIG_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_pmdvalafterpmdp_huge_clear_flush(), then handle the PMD metadata the same waymove_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")Signed-off-by Chris Down <[email protected]>--- mm/huge_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)mm/huge_memory.c +0
-0
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
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 ;-)
David Hildenbrand (Arm)
maintainerQuoted 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
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
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 sovm_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 aspmd_special()on architectures withCONFIG_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_pmdvalafterpmdp_huge_clear_flush(), then handle the PMD metadata the same waymove_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")Signed-off-by Chris Down <[email protected]>--- mm/huge_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)mm/huge_memory.c +0
-0
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
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 sovm_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 aspmd_special()on architectures withCONFIG_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_pmdvalafterpmdp_huge_clear_flush(), then handle the PMD metadata the same waymove_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")Signed-off-by Chris Down <[email protected]>--- mm/huge_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)mm/huge_memory.c +0
-0mm/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:
e3981db444a0with 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
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.