[PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[Li Zhe]

huge_pmd_share() always takes i_mmap_rwsem for read before looking for a
shareable PMD page table.

That is unsafe for callers that already hold the same mapping lock for
write.  In particular, move_hugetlb_page_tables() takes i_mmap_rwsem for
write to prevent truncation races, then calls huge_pte_alloc() for the
destination address.  If the destination PUD is empty and PMD sharing is
possible, huge_pte_alloc() can call huge_pmd_share(), which then tries to
take the same rwsem for read and can deadlock on itself.

PMD sharing is only an optimization.  If the read side of i_mmap_rwsem
cannot be acquired immediately, fall back to allocating a private PMD
table instead of blocking in the sharing path.

Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
---
 mm/hugetlb.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index cea25773a6c95..75631101a6807 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6995,7 +6995,15 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
 	pte_t *spte = NULL;
 	pte_t *pte;
 
-	i_mmap_lock_read(mapping);
+	/*
+	 * Some callers already hold i_mmap_rwsem for write, for example
+	 * move_hugetlb_page_tables(). PMD sharing is only an optimization, so
+	 * fall back to a private PMD table instead of blocking on the same
+	 * rwsem in read mode.
+	 */
+	if (!i_mmap_trylock_read(mapping))
+		goto alloc;
+
 	mapping_rmap_tree_foreach(svma, mapping, idx, idx) {
 		if (svma == vma)
 			continue;
@@ -7024,8 +7032,10 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
 	}
 	spin_unlock(&mm->page_table_lock);
 out:
-	pte = (pte_t *)pmd_alloc(mm, pud, addr);
 	i_mmap_unlock_read(mapping);
+
+alloc:
+	pte = (pte_t *)pmd_alloc(mm, pud, addr);
 	return pte;
 }
 
-- 
2.20.1

Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[Jose A. Perez de Azpillaga]

On Wed, Sep 30, 2026 at 02:43:08PM +0800, Li Zhe wrote:
> huge_pmd_share() always takes i_mmap_rwsem for read before looking for a
> shareable PMD page table.
>
> That is unsafe for callers that already hold the same mapping lock for
> write.  In particular, move_hugetlb_page_tables() takes i_mmap_rwsem for
> write to prevent truncation races, then calls huge_pte_alloc() for the
> destination address.  If the destination PUD is empty and PMD sharing is
> possible, huge_pte_alloc() can call huge_pmd_share(), which then tries to
> take the same rwsem for read and can deadlock on itself.
>
> PMD sharing is only an optimization.  If the read side of i_mmap_rwsem
> cannot be acquired immediately, fall back to allocating a private PMD
> table instead of blocking in the sharing path.
>
> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
> ---
>  mm/hugetlb.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cea25773a6c95..75631101a6807 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6995,7 +6995,15 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	pte_t *spte = NULL;
>  	pte_t *pte;
>
> -	i_mmap_lock_read(mapping);
> +	/*
> +	 * Some callers already hold i_mmap_rwsem for write, for example
> +	 * move_hugetlb_page_tables(). PMD sharing is only an optimization, so
> +	 * fall back to a private PMD table instead of blocking on the same
> +	 * rwsem in read mode.
> +	 */
> +	if (!i_mmap_trylock_read(mapping))
> +		goto alloc;

try deadlock is not a fallback here. both callers already hold the write
lock, and that is enough for the walk, so sharing is never attempted on
these paths.

hugetlb_change_protection() does thr same thing on the uffd-wp path and
is not in the changelog.

and no Fixes tag either. does this want Cc: stable?

>  	mapping_rmap_tree_foreach(svma, mapping, idx, idx) {
>  		if (svma == vma)
>  			continue;
> @@ -7024,8 +7032,10 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	}
>  	spin_unlock(&mm->page_table_lock);
>  out:
> -	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>  	i_mmap_unlock_read(mapping);
> +
> +alloc:
> +	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>  	return pte;
>  }
>
> --
> 2.20.1
>

--
cheers,
jose a. p-a

Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[David Hildenbrand (Arm)]

On 9/30/26 08:43, Li Zhe wrote:
> huge_pmd_share() always takes i_mmap_rwsem for read before looking for a
> shareable PMD page table.
> 
> That is unsafe for callers that already hold the same mapping lock for
> write.  In particular, move_hugetlb_page_tables() takes i_mmap_rwsem for
> write to prevent truncation races, then calls huge_pte_alloc() for the
> destination address.  If the destination PUD is empty and PMD sharing is
> possible, huge_pte_alloc() can call huge_pmd_share(), which then tries to
> take the same rwsem for read and can deadlock on itself.
> 
> PMD sharing is only an optimization.  If the read side of i_mmap_rwsem
> cannot be acquired immediately, fall back to allocating a private PMD
> table instead of blocking in the sharing path.
> 
> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
> ---
>  mm/hugetlb.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index cea25773a6c95..75631101a6807 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6995,7 +6995,15 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>  	pte_t *spte = NULL;
>  	pte_t *pte;
>  
> -	i_mmap_lock_read(mapping);
> +	/*
> +	 * Some callers already hold i_mmap_rwsem for write, for example
> +	 * move_hugetlb_page_tables(). PMD sharing is only an optimization, so
> +	 * fall back to a private PMD table instead of blocking on the same
> +	 * rwsem in read mode.
> +	 */

"just an optimization" is not entirely true. There are workloads that depend on
huge pmd sharing to work. So I don't think this is acceptable.

There was a private discussion on this issue recently (for some weird reason the
deadlock was reported as a security issue), 2 months ago I think.

Oscar, I recall that you wanted to send a proper patch?

-- 
Cheers,

David

Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[Li Zhe]

On 9/30/26 5:31 PM, Jose A. Perez de Azpillaga wrote:
> On Wed, Sep 30, 2026 at 02:43:08PM +0800, Li Zhe wrote:
>> huge_pmd_share() always takes i_mmap_rwsem for read before looking for a
>> shareable PMD page table.
>>
>> That is unsafe for callers that already hold the same mapping lock for
>> write.  In particular, move_hugetlb_page_tables() takes i_mmap_rwsem for
>> write to prevent truncation races, then calls huge_pte_alloc() for the
>> destination address.  If the destination PUD is empty and PMD sharing is
>> possible, huge_pte_alloc() can call huge_pmd_share(), which then tries to
>> take the same rwsem for read and can deadlock on itself.
>>
>> PMD sharing is only an optimization.  If the read side of i_mmap_rwsem
>> cannot be acquired immediately, fall back to allocating a private PMD
>> table instead of blocking in the sharing path.
>>
>> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
>> ---
>>   mm/hugetlb.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index cea25773a6c95..75631101a6807 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -6995,7 +6995,15 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>>   	pte_t *spte = NULL;
>>   	pte_t *pte;
>>
>> -	i_mmap_lock_read(mapping);
>> +	/*
>> +	 * Some callers already hold i_mmap_rwsem for write, for example
>> +	 * move_hugetlb_page_tables(). PMD sharing is only an optimization, so
>> +	 * fall back to a private PMD table instead of blocking on the same
>> +	 * rwsem in read mode.
>> +	 */
>> +	if (!i_mmap_trylock_read(mapping))
>> +		goto alloc;
> try deadlock is not a fallback here. both callers already hold the write
> lock, and that is enough for the walk, so sharing is never attempted on
> these paths.
>
> hugetlb_change_protection() does thr same thing on the uffd-wp path and
> is not in the changelog.
>
> and no Fixes tag either. does this want Cc: stable?


Thanks for the review.

I agree that holding the write lock should be enough for the walk, and
that avoiding PMD sharing on these paths is not the right direction.

David mentioned that Oscar was planning to send a proper fix for this
issue, so I will wait for that patch instead of moving forward with this
trylock fallback approach.

Thanks,
Zhe

>
>>   	mapping_rmap_tree_foreach(svma, mapping, idx, idx) {
>>   		if (svma == vma)
>>   			continue;
>> @@ -7024,8 +7032,10 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>>   	}
>>   	spin_unlock(&mm->page_table_lock);
>>   out:
>> -	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>>   	i_mmap_unlock_read(mapping);
>> +
>> +alloc:
>> +	pte = (pte_t *)pmd_alloc(mm, pud, addr);
>>   	return pte;
>>   }
>>
>> --
>> 2.20.1
>>
> --
> cheers,
> jose a. p-a

Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[David Hildenbrand (Arm)]

On 10/5/26 11:03, Li Zhe wrote:
> On 9/30/26 5:31 PM, Jose A. Perez de Azpillaga wrote:
>> On Wed, Sep 30, 2026 at 02:43:08PM +0800, Li Zhe wrote:
>>> huge_pmd_share() always takes i_mmap_rwsem for read before looking for a
>>> shareable PMD page table.
>>>
>>> That is unsafe for callers that already hold the same mapping lock for
>>> write.  In particular, move_hugetlb_page_tables() takes i_mmap_rwsem for
>>> write to prevent truncation races, then calls huge_pte_alloc() for the
>>> destination address.  If the destination PUD is empty and PMD sharing is
>>> possible, huge_pte_alloc() can call huge_pmd_share(), which then tries to
>>> take the same rwsem for read and can deadlock on itself.
>>>
>>> PMD sharing is only an optimization.  If the read side of i_mmap_rwsem
>>> cannot be acquired immediately, fall back to allocating a private PMD
>>> table instead of blocking in the sharing path.
>>>
>>> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
>>> ---
>>>   mm/hugetlb.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index cea25773a6c95..75631101a6807 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -6995,7 +6995,15 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct vm_area_struct *vma,
>>>   	pte_t *spte = NULL;
>>>   	pte_t *pte;
>>>
>>> -	i_mmap_lock_read(mapping);
>>> +	/*
>>> +	 * Some callers already hold i_mmap_rwsem for write, for example
>>> +	 * move_hugetlb_page_tables(). PMD sharing is only an optimization, so
>>> +	 * fall back to a private PMD table instead of blocking on the same
>>> +	 * rwsem in read mode.
>>> +	 */
>>> +	if (!i_mmap_trylock_read(mapping))
>>> +		goto alloc;
>> try deadlock is not a fallback here. both callers already hold the write
>> lock, and that is enough for the walk, so sharing is never attempted on
>> these paths.
>>
>> hugetlb_change_protection() does thr same thing on the uffd-wp path and
>> is not in the changelog.
>>
>> and no Fixes tag either. does this want Cc: stable?
> 
> 
> Thanks for the review.
> 
> I agree that holding the write lock should be enough for the walk, and
> that avoiding PMD sharing on these paths is not the right direction.
> 
> David mentioned that Oscar was planning to send a proper fix for this
> issue, so I will wait for that patch instead of moving forward with this
> trylock fallback approach.
@Lorenzo, if Oscar is too busy, I guess we can paste the overall idea for the
fix here as well (publicly)?

-- 
Cheers,

David