Re: [PATCH] mm/khugepaged: fix collapse_pte_mapped_thp() to allow anon_vma

[David Hildenbrand]

On 22.12.22 21:41, Hugh Dickins wrote:
> uprobe_write_opcode() uses collapse_pte_mapped_thp() to restore huge pmd,
> when removing a breakpoint from hugepage text: vma->anon_vma is always
> set in that case, so undo the prohibition.  And MADV_COLLAPSE ought to be
> able to collapse some page tables in a vma which happens to have anon_vma
> set from CoWing elsewhere.

Just so I get this correctly: the degradation is that we won't 
re-collapse a THP after removing a breakpoint. Certainly "sub-optimal", 
but I guess nothing that particularly matters in practice.

Or am I wrong?

> Is anon_vma lock required?  Almost not: if any page other than expected
> subpage of the non-anon huge page is found in the page table, collapse is
> aborted without making any change.  However, it is possible that an anon
> page was CoWed from this extent in another mm or vma, in which case a
> concurrent lookup might look here: so keep it away while clearing pmd
> (but perhaps we shall go back to using pmd_lock() there in future).
>
> Note that collapse_pte_mapped_thp() is exceptional in freeing a page table
> without having cleared its ptes: I'm uneasy about that, and had thought
> pte_clear()ing appropriate; but exclusive i_mmap lock does fix the problem,
> and we would have to move the mmu_notification if clearing those ptes.
>
> Fixes: 8d3c106e19e8 ("mm/khugepaged: take the right locks for page table retraction")
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Jann Horn <jannh@xxxxxxxxxx>
> Cc: Yang Shi <shy828301@xxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Zach O'Keefe <zokeefe@xxxxxxxxxx>
> Cc: Song Liu <songliubraving@xxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>    [5.4+]
> ---
> What this fixes is not a dangerous instability!  But I suggest Cc stable
> because uprobes "healing" has regressed in that way, so this should follow
> 8d3c106e19e8 into those stable releases where it was backported (and may
> want adjustment there - I'll supply backports as needed).

If it's really something that doesn't matter in practice (e.g., -1% 
performance while debugging :) ), I guess no CC is needed. If there are 
real production workloads that suffer, I guess ccing stable is fine.

>   mm/khugepaged.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
>
> --- 6.2-rc/mm/khugepaged.c
> +++ linux/mm/khugepaged.c
> @@ -1460,14 +1460,6 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
>   	if (!hugepage_vma_check(vma, vma->vm_flags, false, false, false))
>   		return SCAN_VMA_CHECK;
>   
> -	/*
> -	 * Symmetry with retract_page_tables(): Exclude MAP_PRIVATE mappings
> -	 * that got written to. Without this, we'd have to also lock the
> -	 * anon_vma if one exists.
> -	 */
> -	if (vma->anon_vma)
> -		return SCAN_VMA_CHECK;
> -
>   	/* Keep pmd pgtable for uffd-wp; see comment in retract_page_tables() */
>   	if (userfaultfd_wp(vma))
>   		return SCAN_PTE_UFFD_WP;
> @@ -1567,8 +1559,14 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
>   	}
>   
>   	/* step 4: remove pte entries */
> +	/* we make no change to anon, but protect concurrent anon page lookup */
> +	if (vma->anon_vma)
> +		anon_vma_lock_write(vma->anon_vma);
> +
>   	collapse_and_free_pmd(mm, vma, haddr, pmd);
>   
> +	if (vma->anon_vma)
> +		anon_vma_unlock_write(vma->anon_vma);
>   	i_mmap_unlock_write(vma->vm_file->f_mapping);
>   
>   maybe_install_pmd:

That code is 99% black magic to me, but staring at the original fix and 
at collapse_and_free_pmd() makes me assume that grabbing that lock most 
certainly won't hurt and should be the right thing to do

Acked-by: David Hildenbrand <david@xxxxxxxxxx>


Side note: set_huge_pmd() wins the award of "ugliest mm function of 
early 2023". I was briefly concerned how do_set_pmd() decides whether 
the PMD can be writable or not. Turns out it's communicated via 
vm_fault->flags. Just horrible.

--
Thanks,

David / dhildenb