Re: [PATCH] mm/hugetlb: Fix pgtable lock on pmd sharing

[Mike Kravetz]

On 06/12/23 12:04, Peter Xu wrote:
> Huge pmd sharing operates on PUD not PMD, huge_pte_lock() is not suitable
> in this case because it should only work for last level pte changes, while
> pmd sharing is always one level higher.

Right!  That lock does not prevent someone else from concurrently modifying
the PUD.

> Meanwhile, here we're locking over the spte pgtable lock which is even not
> a lock for current mm but someone else's.
> 
> It seems even racy on operating on the lock, as after put_page() of the
> spte pgtable page logically the page can be released, so at least the
> spin_unlock() needs to be done after the put_page().

Agree.

> No report I am aware, I'm not even sure whether it'll just work on taking
> the spte pmd lock, because while we're holding i_mmap read lock it probably
> means the vma interval tree is frozen, all pte allocators over this pud
> entry could always find the specific svma and spte page, so maybe they'll
> serialize on this spte page lock?

It seems they would serialize IF they were trying to instantiate the same
shared page.  However, I suppose another thread could be trying to
instantiate something totally different in the VA range represented by that
PUD.  In this case, it seems like there would be no synchronization.

>                                    Even so, doesn't seem to be expected.
> It just seems to be an accident of cb900f412154.
> 
> Fix it with the proper pud lock (which is the mm's page_table_lock).
> 
> Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> Cc: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> Fixes: cb900f412154 ("mm, hugetlb: convert hugetlbfs to use split pmd lock")
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
>  mm/hugetlb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Agree with this change.  But, it does make one wonder if the pud_clear()
in huge_pmd_unshare is safe.  Like here, we really should be holding the
higher level lock but are holding the PMD lock.
-- 
Mike Kravetz

> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index dfa412d8cb30..270ec0ecd5a1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -7133,7 +7133,6 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	unsigned long saddr;
>  	pte_t *spte = NULL;
>  	pte_t *pte;
> -	spinlock_t *ptl;
>  
>  	i_mmap_lock_read(mapping);
>  	vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
> @@ -7154,7 +7153,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	if (!spte)
>  		goto out;
>  
> -	ptl = huge_pte_lock(hstate_vma(vma), mm, spte);
> +	spin_lock(&mm->page_table_lock);
>  	if (pud_none(*pud)) {
>  		pud_populate(mm, pud,
>  				(pmd_t *)((unsigned long)spte & PAGE_MASK));
> @@ -7162,7 +7161,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	} else {
>  		put_page(virt_to_page(spte));
>  	}
> -	spin_unlock(ptl);
> +	spin_unlock(&mm->page_table_lock);
>  out:
>  	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>  	i_mmap_unlock_read(mapping);
> -- 
> 2.40.1
>