Re: [PATCH v4 12/17] mm: remember exclusively mapped anonymous pages with PG_anon_exclusive

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/4/28 16:34, David Hildenbrand wrote:
> Let's mark exclusively mapped anonymous pages with PG_anon_exclusive as
> exclusive, and use that information to make GUP pins reliable and stay
> consistent with the page mapped into the page table even if the
> page table entry gets write-protected.
> 
> With that information at hand, we can extend our COW logic to always
> reuse anonymous pages that are exclusive. For anonymous pages that
> might be shared, the existing logic applies.
> 
> As already documented, PG_anon_exclusive is usually only expressive in
> combination with a page table entry. Especially PTE vs. PMD-mapped
> anonymous pages require more thought, some examples: due to mremap() we
> can easily have a single compound page PTE-mapped into multiple page tables
> exclusively in a single process -- multiple page table locks apply.
> Further, due to MADV_WIPEONFORK we might not necessarily write-protect
> all PTEs, and only some subpages might be pinned. Long story short: once
> PTE-mapped, we have to track information about exclusivity per sub-page,
> but until then, we can just track it for the compound page in the head
> page and not having to update a whole bunch of subpages all of the time
> for a simple PMD mapping of a THP.
> 
> For simplicity, this commit mostly talks about "anonymous pages", while
> it's for THP actually "the part of an anonymous folio referenced via
> a page table entry".
> 
> To not spill PG_anon_exclusive code all over the mm code-base, we let
> the anon rmap code to handle all PG_anon_exclusive logic it can easily
> handle.
> 
> If a writable, present page table entry points at an anonymous (sub)page,
> that (sub)page must be PG_anon_exclusive. If GUP wants to take a reliably
> pin (FOLL_PIN) on an anonymous page references via a present
> page table entry, it must only pin if PG_anon_exclusive is set for the
> mapped (sub)page.
> 
> This commit doesn't adjust GUP, so this is only implicitly handled for
> FOLL_WRITE, follow-up commits will teach GUP to also respect it for
> FOLL_PIN without FOLL_WRITE, to make all GUP pins of anonymous pages
> fully reliable.
> 
> Whenever an anonymous page is to be shared (fork(), KSM), or when
> temporarily unmapping an anonymous page (swap, migration), the relevant
> PG_anon_exclusive bit has to be cleared to mark the anonymous page
> possibly shared. Clearing will fail if there are GUP pins on the page:
> * For fork(), this means having to copy the page and not being able to
>   share it. fork() protects against concurrent GUP using the PT lock and
>   the src_mm->write_protect_seq.
> * For KSM, this means sharing will fail. For swap this means, unmapping
>   will fail, For migration this means, migration will fail early. All
>   three cases protect against concurrent GUP using the PT lock and a
>   proper clear/invalidate+flush of the relevant page table entry.
> 
> This fixes memory corruptions reported for FOLL_PIN | FOLL_WRITE, when a
> pinned page gets mapped R/O and the successive write fault ends up
> replacing the page instead of reusing it. It improves the situation for
> O_DIRECT/vmsplice/... that still use FOLL_GET instead of FOLL_PIN,
> if fork() is *not* involved, however swapout and fork() are still
> problematic. Properly using FOLL_PIN instead of FOLL_GET for these
> GUP users will fix the issue for them.
> 

Hi David, sorry for the late respond and a possible inconsequential question. :)

<snip>

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7a71ed679853..5add8bbd47cd 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4772,7 +4772,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  				    is_hugetlb_entry_hwpoisoned(entry))) {
>  			swp_entry_t swp_entry = pte_to_swp_entry(entry);
>  
> -			if (is_writable_migration_entry(swp_entry) && cow) {
> +			if (!is_readable_migration_entry(swp_entry) && cow) {
>  				/*
>  				 * COW mappings require pages in both
>  				 * parent and child to be set to read.
> @@ -5172,6 +5172,8 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
>  		set_huge_ptep_writable(vma, haddr, ptep);
>  		return 0;
>  	}
> +	VM_BUG_ON_PAGE(PageAnon(old_page) && PageAnonExclusive(old_page),
> +		       old_page);
>  
>  	/*
>  	 * If the process that created a MAP_PRIVATE mapping is about to
> @@ -6169,12 +6171,17 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
>  		}
>  		if (unlikely(is_hugetlb_entry_migration(pte))) {
>  			swp_entry_t entry = pte_to_swp_entry(pte);
> +			struct page *page = pfn_swap_entry_to_page(entry);
>  
> -			if (is_writable_migration_entry(entry)) {
> +			if (!is_readable_migration_entry(entry)) {

In hugetlb_change_protection(), is_writable_migration_entry() is changed to !is_readable_migration_entry(),
but

>  				pte_t newpte;
>  
> -				entry = make_readable_migration_entry(
> -							swp_offset(entry));
> +				if (PageAnon(page))
> +					entry = make_readable_exclusive_migration_entry(
> +								swp_offset(entry));
> +				else
> +					entry = make_readable_migration_entry(
> +								swp_offset(entry));
>  				newpte = swp_entry_to_pte(entry);
>  				set_huge_swap_pte_at(mm, address, ptep,
>  						     newpte, huge_page_size(h));

<snip>

> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index b69ce7a7b2b7..56060acdabd3 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -152,6 +152,7 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
>  			pages++;
>  		} else if (is_swap_pte(oldpte)) {
>  			swp_entry_t entry = pte_to_swp_entry(oldpte);
> +			struct page *page = pfn_swap_entry_to_page(entry);
>  			pte_t newpte;
>  
>  			if (is_writable_migration_entry(entry)) {

In change_pte_range(), is_writable_migration_entry() is not changed to !is_readable_migration_entry().
Is this done intentionally? Could you tell me why there's such a difference? I'm confused. It's very
kind of you if you can answer my puzzle.

Thanks!
Miaohe Lin

> @@ -159,8 +160,11 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
>  				 * A protection check is difficult so
>  				 * just be safe and disable write
>  				 */
> -				entry = make_readable_migration_entry(
> -							swp_offset(entry));
> +				if (PageAnon(page))
> +					entry = make_readable_exclusive_migration_entry(
> +							     swp_offset(entry));
> +				else
> +					entry = make_readable_migration_entry(swp_offset(entry));
>  				newpte = swp_entry_to_pte(entry);
>  				if (pte_swp_soft_dirty(oldpte))
>  					newpte = pte_swp_mksoft_dirty(newpte);