Re: [PATCH] hugetlb: retry private VMA locking when unmapping folios

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/15/26 19:05, Daehyeon Ko wrote:
> hugetlb_unmap_file_folio() takes a reference to the hugetlb_vma_lock in
> vm_private_data when its VMA trylock fails. It then drops i_mmap_rwsem and
> waits for the lock in the established order.
> 
> That fallback is only valid for VM_MAYSHARE VMAs. Since commit
> bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs"), the
> trylock can also fail for a private owner VMA. Its vm_private_data contains
> resv_map | HPAGE_RESV_OWNER, not a hugetlb_vma_lock. The fallback then
> performs a misaligned refcount operation and treats reservation-map list
> fields as an rwsem.
> 
> Private reservation maps have no stable VMA backpointer. Drop i_mmap_rwsem
> and restart the interval-tree walk when their trylock fails. Keep the
> referenced fallback for shareable VMAs.
> 
> On a system with a pre-provisioned hugepage pool, an unprivileged
> reproducer racing mprotect() with hole punching triggered the refcount
> warning followed by a general protection fault in rwsem_down_write_slowpath
> on v7.3-rc5 and 6.12.111. With this change, a v7.3-rc5 KASAN+LOCKDEP build
> completed 1,000 rounds and 64,000 hole punches without a warning, lockdep
> report, oops, or panic. The reproducer is available privately on request.
> 
> Fixes: bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>
> ---
>  fs/hugetlbfs/inode.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 7611a8470ea265..9a676228e2070a 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -402,6 +402,17 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  			continue;
>  
>  		if (!hugetlb_vma_trylock_write(vma)) {
> +			/*
> +			 * The referenced fallback below is specific to the
> +			 * hugetlb_vma_lock stored by shareable VMAs.  A private
> +			 * owner VMA stores a tagged resv_map pointer here instead.
> +			 */
> +			if (!(vma->vm_flags & VM_MAYSHARE)) {
> +				i_mmap_unlock_write(mapping);
> +				cond_resched();
> +				i_mmap_lock_write(mapping);
> +				goto retry;

That looks like we can just live-lock now?

> +			}
>  			vma_lock = vma->vm_private_data;


This whole function is a mess.

I have the following AI fix in a queue-of-fixes to go through and send out.

The description is a bit mouthful, but the fix itself looks much cleaner (just skip the
write-lock for VMAs that don't need it):


Does that make sense?


commit 12a35d8ee3e638129080c2bb13c0a35c873b5257
Author: Josef Bacik <josef@toxicpanda.com>
Date:   Thu Oct 8 06:55:26 2026 +0000

    hugetlbfs: fix unmapping a folio from a private vma

    hugetlb_unmap_file_folio() unmaps a folio which is about to leave the
    page cache from every vma that maps it.  Where it can not get the lock of
    a vma with hugetlb_vma_trylock_write(), it takes vma->vm_private_data for
    a struct hugetlb_vma_lock, gets a reference to it, drops i_mmap_rwsem and
    sleeps on its rw_sema.

    Since commit bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private
    VMAs") the trylock can fail for a MAP_PRIVATE vma that owns a reservation
    map, too.  The lock of such a vma is resv_map->rw_sema, which every fault
    in the vma holds for reading, and its vm_private_data is the address of
    the resv_map with HPAGE_RESV_OWNER in bit 0.  kref_get() and down_write()
    then work on the bytes of the resv_map, one byte off.

    A private vma maps a folio of the page cache after a read fault.  Three
    threads of an unprivileged process are enough, on a hugetlb memfd: one
    reads a page through a private mapping, one writes to another page of
    that mapping, one allocates the first page with fallocate() and punches
    it out again.  ftruncate() and MADV_REMOVE do as well as the hole punch.

      refcount_t: addition on 0; use-after-free.
      WARNING: lib/refcount.c:25 at refcount_warn_saturate+0x6a/0x90
       remove_inode_hugepages+0x49d/0x530
       hugetlbfs_fallocate+0x4bf/0x5f0
       vfs_fallocate+0x13d/0x330

    On x86-64, saturating the "reference count" stores 0xc0 into the first
    byte of resv_map->lock: the next fault in the vma spins in region_chg()
    for ever.  down_write() either takes a list pointer, shifted by one byte,
    for the owner and dies of a general protection fault, or spins with
    interrupts off on what it takes for the wait_lock, in both cases with the
    fault mutex and the inode lock held.

    Leave the lock of a vma that is not shareable alone, as it was before that
    commit.  That lock keeps faults out of the vma until MADV_DONTNEED has
    freed its pages.  It is not needed here: the caller holds the fault mutex
    of the index, so nobody can map the folio or replace it by a copy, and
    i_mmap_rwsem, held for writing, keeps the vma in the file's tree and its
    page tables in place.

    Fixes: bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs")
    Assisted-by: LLM
    Signed-off-by: Josef Bacik <josef@toxicpanda.com>

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index d21dbcdc705ed..02cec281ceba5 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -401,6 +401,19 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
                if (!hugetlb_vma_maps_pfn(vma, v_start, pfn))
                        continue;

+               /*
+                * Only a shareable vma has a struct hugetlb_vma_lock.  The
+                * lock of a private vma lives in its resv_map and keeps
+                * faults away from MADV_DONTNEED; it is not needed here:
+                * the fault mutex holds off faults on this index, and
+                * i_mmap_rwsem everybody who zaps or frees page tables.
+                */
+               if (!__vma_shareable_lock(vma)) {
+                       unmap_hugepage_range(vma, v_start, v_end, NULL,
+                                            ZAP_FLAG_DROP_MARKER);
+                       continue;
+               }
+
                if (!hugetlb_vma_trylock_write(vma)) {
                        vma_lock = vma->vm_private_data;
                        /*

-- 
Cheers,

David