Re: [PATCH v2] mm/hugetlb: fix a deadlock with pagecache_folio and hugetlb_fault_mutex_table

[Hugh Dickins <hughd@google.com>]

On Wed, 21 May 2025, Gavin Guo wrote:
>...
> V1 -> V2
> Suggested-by Oscar Salvador:
>   - Use folio_test_locked to replace the unnecessary parameter passing.
> 
>  mm/hugetlb.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7ae38bfb9096..ed501f134eff 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6226,6 +6226,12 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio,
>  			u32 hash;
>  
>  			folio_put(old_folio);
> +			/*
> +			 * The pagecache_folio needs to be unlocked to avoid
> +			 * deadlock when the child unmaps the folio.
> +			 */
> +			if (pagecache_folio)
> +				folio_unlock(pagecache_folio);
>  			/*
>  			 * Drop hugetlb_fault_mutex and vma_lock before
>  			 * unmapping.  unmapping needs to hold vma_lock
> @@ -6823,8 +6829,13 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  out_ptl:
>  	spin_unlock(vmf.ptl);
>  
> +	/*
> +	 * hugetlb_wp() might have already unlocked pagecache_folio, so
> +	 * skip it if that is the case.
> +	 */
>  	if (pagecache_folio) {
> -		folio_unlock(pagecache_folio);
> +		if (folio_test_locked(pagecache_folio))
> +			folio_unlock(pagecache_folio);
>  		folio_put(pagecache_folio);
>  	}
>  out_mutex:

NAK!

I have not (and shall not) review V1, but was hoping someone else
would save me from rejecting this V2 idea immediately.

Unless you have a very strong argument why this folio is invisible to
the rest of the world, including speculative accessors like compaction
(and the name "pagecache_folio" suggests very much the reverse): the
pattern of unlocking a lock when you see it locked is like (or worse
than) having no locking at all - it is potentially unlocking someone
else's lock.

Hugh