Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dc9f7bb0-77d4-8b0e-c6b6-ece3f77ed326@redhat.com>
Date:   Mon, 2 Jan 2023 13:19:26 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Content-Language: en-US
To:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     Jann Horn <jannh@google.com>, Yang Shi <shy828301@gmail.com>,
        Zach O'Keefe <zokeefe@google.com>,
        Song Liu <songliubraving@fb.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
References: <b740c9fb-edba-92ba-59fb-7a5592e5dfc@google.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH] mm/khugepaged: fix collapse_pte_mapped_thp() to allow
 anon_vma
In-Reply-To: <b740c9fb-edba-92ba-59fb-7a5592e5dfc@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@google.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Zach O'Keefe <zokeefe@google.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: <stable@vger.kernel.org>    [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@redhat.com>


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