linux-kernel - Re: [PATCH v2] mm/hugetlb: fix a deadlock with pagecache_folio and hugetlb_fault_mutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30681817-6820-6b43-1f39-065c5f1b3596@google.com>
Date: Wed, 21 May 2025 08:10:46 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Gavin Guo <gavinguo@...lia.com>
cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, muchun.song@...ux.dev, 
    osalvador@...e.de, akpm@...ux-foundation.org, mike.kravetz@...cle.com, 
    kernel-dev@...lia.com, stable@...r.kernel.org, 
    Hugh Dickins <hughd@...gle.com>, Florent Revest <revest@...gle.com>, 
    Gavin Shan <gshan@...hat.com>
Subject: Re: [PATCH v2] mm/hugetlb: fix a deadlock with pagecache_folio and
 hugetlb_fault_mutex_table

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