[PATCH for Sashiko] hugetlb: supports unmapping private mappings

[PATCH for Sashiko] hugetlb: supports unmapping private mappings

[Edward Adam Davis]

Note!
This patch only for Sashiko.

If a file has a private mapping, then truncating the file's original logic
will only unmap it according to the shared mapping method, potentially
hitting:

refcount_t: addition on 0; use-after-free.
kernel BUG at lib/refcount.c:34!
RIP: 0010:refcount_warn_saturate+0xf0/0x110 lib/refcount.c:34
Call Trace:
 kref_get include/linux/kref.h:45 [inline]
 hugetlb_unmap_file_folio fs/hugetlbfs/inode.c:413 [inline]
 remove_inode_single_folio fs/hugetlbfs/inode.c:512 [inline]
 remove_inode_hugepages+0xbdb/0x10f0 fs/hugetlbfs/inode.c:577
 hugetlb_vmtruncate fs/hugetlbfs/inode.c:621 [inline]
 hugetlbfs_setattr+0x4f0/0x6d0 fs/hugetlbfs/inode.c:856
 notify_change+0xbb8/0xec0 fs/attr.c:556
 do_truncate+0x1c2/0x250 fs/open.c:68

Add logic to handle the unmapping of private mappings.

Fixes: 40549ba8f8e0 ("hugetlb: use new vma_lock for pmd sharing synchronization")
Reported-by: syzbot+2fcec0eca7f4cade53b2@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2fcec0eca7f4cade53b2
Signed-off-by: Edward Adam Davis <eadavis@sina.com>
---
 fs/hugetlbfs/inode.c    | 58 +++++++++++++++++++++++++----------------
 include/linux/hugetlb.h |  6 +++++
 mm/hugetlb.c            |  3 +--
 3 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 6656807ce437..166183b7ee36 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -382,6 +382,7 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
 					struct folio *folio, pgoff_t index)
 {
 	struct hugetlb_vma_lock *vma_lock;
+	struct resv_map *resv_map;
 	unsigned long pfn = folio_pfn(folio);
 	struct vm_area_struct *vma;
 	unsigned long v_start;
@@ -394,6 +395,7 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
 	i_mmap_lock_write(mapping);
 retry:
 	vma_lock = NULL;
+	resv_map = NULL;
 	mapping_rmap_tree_foreach(vma, mapping, start, end - 1) {
 		v_start = vma_offset_start(vma, start);
 		v_end = vma_offset_end(vma, end);
@@ -402,15 +404,21 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
 			continue;
 
 		if (!hugetlb_vma_trylock_write(vma)) {
-			vma_lock = vma->vm_private_data;
-			/*
-			 * If we can not get vma lock, we need to drop
-			 * immap_sema and take locks in order.  First,
-			 * take a ref on the vma_lock structure so that
-			 * we can be guaranteed it will not go away when
-			 * dropping immap_sema.
-			 */
-			kref_get(&vma_lock->refs);
+			if (vma->vm_flags & VM_MAYSHARE) {
+				vma_lock = vma->vm_private_data;
+				/*
+				 * If we can not get vma lock, we need to drop
+				 * immap_sema and take locks in order.  First,
+				 * take a ref on the vma_lock structure so that
+				 * we can be guaranteed it will not go away when
+				 * dropping immap_sema.
+				 */
+				kref_get(&vma_lock->refs);
+			} else {
+				resv_map = vma_resv_map(vma);
+				kref_get(&resv_map->refs);
+			}
+
 			break;
 		}
 
@@ -421,25 +429,28 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
 
 	i_mmap_unlock_write(mapping);
 
-	if (vma_lock) {
+	if (vma_lock || resv_map) {
 		/*
 		 * Wait on vma_lock.  We know it is still valid as we have
 		 * a reference.  We must 'open code' vma locking as we do
 		 * not know if vma_lock is still attached to vma.
 		 */
-		down_write(&vma_lock->rw_sema);
+		hugetlb_vma_lock_write(vma);
 		i_mmap_lock_write(mapping);
 
-		vma = vma_lock->vma;
-		if (!vma) {
-			/*
-			 * If lock is no longer attached to vma, then just
-			 * unlock, drop our reference and retry looking for
-			 * other vmas.
-			 */
-			up_write(&vma_lock->rw_sema);
-			kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
-			goto retry;
+		if (vma_lock) {
+			vma = vma_lock->vma;
+			if (!vma) {
+				/*
+				 * If lock is no longer attached to vma, then
+				 * just unlock, drop our reference and retry
+				 * looking for other vmas.
+				 */
+				up_write(&vma_lock->rw_sema);
+				kref_put(&vma_lock->refs,
+					 hugetlb_vma_lock_release);
+				goto retry;
+			}
 		}
 
 		/*
@@ -452,7 +463,10 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
 			unmap_hugepage_range(vma, v_start, v_end, NULL,
 					     ZAP_FLAG_DROP_MARKER);
 
-		kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
+		if (vma_lock)
+			kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
+		else
+			kref_put(&resv_map->refs, resv_map_release);
 		hugetlb_vma_unlock_write(vma);
 
 		goto retry;
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 80a5a03e9cee..f980191c651b 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -268,6 +268,7 @@ void hugetlb_vma_unlock_read(struct vm_area_struct *vma);
 void hugetlb_vma_lock_write(struct vm_area_struct *vma);
 void hugetlb_vma_unlock_write(struct vm_area_struct *vma);
 int hugetlb_vma_trylock_write(struct vm_area_struct *vma);
+struct resv_map *vma_resv_map(struct vm_area_struct *vma);
 void hugetlb_vma_assert_locked(struct vm_area_struct *vma);
 void hugetlb_vma_lock_release(struct kref *kref);
 long hugetlb_change_protection(struct vm_area_struct *vma,
@@ -392,6 +393,11 @@ static inline int is_hugepage_only_range(struct mm_struct *mm,
 	return 0;
 }
 
+static inline struct resv_map *vma_resv_map(struct vm_area_struct *vma)
+{
+	return NULL;
+}
+
 #ifdef CONFIG_USERFAULTFD
 static inline int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 					   struct vm_area_struct *dst_vma,
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b017ae1dfd6f..def6aebc44fd 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -141,7 +141,6 @@ static void hugetlb_unmap_range(struct mmu_gather *tlb,
 				struct vm_area_struct *vma, unsigned long start,
 				unsigned long end, struct folio *folio,
 				zap_flags_t zap_flags);
-static struct resv_map *vma_resv_map(struct vm_area_struct *vma);
 
 static inline bool subpool_is_free(struct hugepage_subpool *spool)
 {
@@ -1152,7 +1152,7 @@ static inline struct resv_map *inode_resv_map(struct inode *inode)
 	return HUGETLBFS_I(inode)->resv_map;
 }
 
-static struct resv_map *vma_resv_map(struct vm_area_struct *vma)
+struct resv_map *vma_resv_map(struct vm_area_struct *vma)
 {
 	VM_WARN_ON_ONCE_VMA(!vma_is_hugetlb(vma), vma);
 	if (vma->vm_flags & VM_MAYSHARE) {
-- 
2.43.0

Re: [PATCH for Sashiko] hugetlb: supports unmapping private mappings

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] hugetlbfs: use-after-free and unbalanced unlock in hugetlb_unmap_file_folio()
- [Critical] hugetlbfs: unvalidated private vma use-after-free in hugetlb_unmap_file_folio()
- [Critical] hugetlbfs: resv_map use-after-free on unlock in hugetlb_unmap_file_folio()
--

commit cc572a12212d775135723329431694314f130c15
Author: Edward Adam Davis <eadavis@sina.com>

hugetlb: supports unmapping private mappings

This commit updates hugetlb_unmap_file_folio() to take a reference on a
private mapping's resv_map instead of assuming vma->vm_private_data points
to a struct hugetlb_vma_lock when hugetlb_vma_trylock_write() fails.

> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 01c4d6f44ebe0..37be669653835 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -381,6 +381,7 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  					struct folio *folio, pgoff_t index)
>  {
>  	struct hugetlb_vma_lock *vma_lock;
> +	struct resv_map *resv_map;
>  	unsigned long pfn = folio_pfn(folio);
>  	struct vm_area_struct *vma;
>  	unsigned long v_start;
> @@ -393,6 +394,7 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  	i_mmap_lock_write(mapping);
>  retry:
>  	vma_lock = NULL;
> +	resv_map = NULL;
>  	mapping_rmap_tree_foreach(vma, mapping, start, end - 1) {
>  		v_start = vma_offset_start(vma, start);
>  		v_end = vma_offset_end(vma, end);
> @@ -401,15 +403,21 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  			continue;
>  
>  		if (!hugetlb_vma_trylock_write(vma)) {
> -			vma_lock = vma->vm_private_data;
> -			/*
> -			 * If we can not get vma lock, we need to drop
> -			 * immap_sema and take locks in order.  First,
> -			 * take a ref on the vma_lock structure so that
> -			 * we can be guaranteed it will not go away when
> -			 * dropping immap_sema.
> -			 */
> -			kref_get(&vma_lock->refs);
> +			if (vma->vm_flags & VM_MAYSHARE) {
> +				vma_lock = vma->vm_private_data;
> +				/*
> +				 * If we can not get vma lock, we need to drop
> +				 * immap_sema and take locks in order.  First,
> +				 * take a ref on the vma_lock structure so that
> +				 * we can be guaranteed it will not go away when
> +				 * dropping immap_sema.
> +				 */
> +				kref_get(&vma_lock->refs);
> +			} else {
> +				resv_map = vma_resv_map(vma);
> +				kref_get(&resv_map->refs);
> +			}
> +
>  			break;
>  		}
>  
> @@ -420,25 +428,28 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  
>  	i_mmap_unlock_write(mapping);
>  
> -	if (vma_lock) {
> +	if (vma_lock || resv_map) {
>  		/*
>  		 * Wait on vma_lock.  We know it is still valid as we have
>  		 * a reference.  We must 'open code' vma locking as we do
>  		 * not know if vma_lock is still attached to vma.
>  		 */
> -		down_write(&vma_lock->rw_sema);
> +		hugetlb_vma_lock_write(vma);

[Severity: Critical]
Does calling hugetlb_vma_lock_write(vma) after i_mmap_unlock_write(mapping)
cause a use-after-free on vma and an unbalanced up_write(&vma_lock->rw_sema)?

In hugetlb_unmap_file_folio(), the caller does not hold vma->vm_mm->mmap_lock,
so vma is only stabilized while i_mmap_lock_write(mapping) is held. Taking a
reference on vma_lock->refs or resv_map->refs does not pin struct
vm_area_struct. Once i_mmap_unlock_write(mapping) drops i_mmap_rwsem, a
concurrent munmap(), exit_mmap(), or mremap() can unlink vma from
mapping->i_mmap and free it via vm_area_free(vma) before
hugetlb_vma_lock_write(vma) reads vma->vm_flags and vma->vm_private_data:

mm/hugetlb.c:hugetlb_vma_lock_write() {
	if (__vma_shareable_lock(vma)) {
		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;

		down_write(&vma_lock->rw_sema);
	} else if (__vma_private_lock(vma)) {
		struct resv_map *resv_map = vma_resv_map(vma);

		down_write(&resv_map->rw_sema);
	}
}

Furthermore, for a shared VMA (vma_lock != NULL), a concurrent final unmap in
__hugetlb_zap_end() -> __hugetlb_vma_unlock_write_free() ->
__hugetlb_vma_unlock_write_put() clears both vma_lock->vma and
vma->vm_private_data before releasing vma_lock->rw_sema:

mm/hugetlb.c:__hugetlb_vma_unlock_write_put() {
	struct vm_area_struct *vma = vma_lock->vma;

	vma_lock->vma = NULL;
	vma->vm_private_data = NULL;
	up_write(&vma_lock->rw_sema);
	kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
}

If vma->vm_private_data is cleared before hugetlb_vma_lock_write(vma) runs,
__vma_shareable_lock(vma) sees vma->vm_private_data == NULL and returns false
without calling down_write(&vma_lock->rw_sema) (or dereferences NULL if
vma->vm_private_data is cleared right after __vma_shareable_lock(vma) checks
it). Then hugetlb_unmap_file_folio() sees vma_lock->vma == NULL and calls
up_write(&vma_lock->rw_sema) on a semaphore it never locked.

>  		i_mmap_lock_write(mapping);
>  
> -		vma = vma_lock->vma;
> -		if (!vma) {
> -			/*
> -			 * If lock is no longer attached to vma, then just
> -			 * unlock, drop our reference and retry looking for
> -			 * other vmas.
> -			 */
> -			up_write(&vma_lock->rw_sema);
> -			kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
> -			goto retry;
> +		if (vma_lock) {
> +			vma = vma_lock->vma;
> +			if (!vma) {
> +				/*
> +				 * If lock is no longer attached to vma, then
> +				 * just unlock, drop our reference and retry
> +				 * looking for other vmas.
> +				 */
> +				up_write(&vma_lock->rw_sema);
> +				kref_put(&vma_lock->refs,
> +					 hugetlb_vma_lock_release);
> +				goto retry;
> +			}
>  		}
>  
>  		/*
> @@ -451,7 +462,10 @@ static void hugetlb_unmap_file_folio(struct hstate *h,
>  			unmap_hugepage_range(vma, v_start, v_end, NULL,
>  					     ZAP_FLAG_DROP_MARKER);

[Severity: Critical]
When unmapping a private VMA (resv_map != NULL and vma_lock == NULL), what
guarantees that vma is still valid after reacquiring i_mmap_lock_write(mapping)?

Unlike struct hugetlb_vma_lock, struct resv_map has no backpointer to vma, and
__hugetlb_vma_unlock_write_free() only unlocks resv_map->rw_sema:

mm/hugetlb.c:__hugetlb_vma_unlock_write_free() {
	if (__vma_shareable_lock(vma)) {
		struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;

		__hugetlb_vma_unlock_write_put(vma_lock);
	} else if (__vma_private_lock(vma)) {
		struct resv_map *resv_map = vma_resv_map(vma);

		/* no free for anon vmas, but still need to unlock */
		up_write(&resv_map->rw_sema);
	}
}

While i_mmap_rwsem is dropped, a concurrent munmap(), exit_mmap(), or mremap()
can finish unmapping the private VMA, unlink it from mapping->i_mmap, free its
page tables, and free vma (and potentially vma->vm_mm). When
hugetlb_unmap_file_folio() reacquires i_mmap_lock_write(mapping), the
if (vma_lock) check is skipped and vma is passed directly to
vma_offset_start(), vma_offset_end(), hugetlb_vma_maps_pfn(),
unmap_hugepage_range(), and hugetlb_vma_unlock_write() without checking
whether vma was unmapped and freed.

In addition, if a concurrent mremap() runs fixup_hugetlb_reservations() ->
clear_vma_resv_huge_pages(vma) under i_mmap_lock_write() before
hugetlb_unmap_file_folio() reacquires i_mmap_lock_write(mapping),
vma->vm_private_data on the old VMA is cleared to NULL while resv_map is
moved to the new VMA. The subsequent hugetlb_vma_unlock_write(vma) will see
__vma_private_lock(vma) == false and skip up_write(&resv_map->rw_sema),
leaving resv_map->rw_sema permanently write-locked.

>  
> -		kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
> +		if (vma_lock)
> +			kref_put(&vma_lock->refs, hugetlb_vma_lock_release);
> +		else
> +			kref_put(&resv_map->refs, resv_map_release);
>  		hugetlb_vma_unlock_write(vma);

[Severity: Critical]
Can kref_put(&resv_map->refs, resv_map_release) free resv_map before
hugetlb_vma_unlock_write(vma) unlocks resv_map->rw_sema?

For private VMAs, hugetlb_vm_op_close() (and clear_vma_resv_huge_pages()) can
drop the VMA's reference on resv_map->refs while i_mmap_rwsem was dropped,
without taking resv_map->rw_sema:

mm/hugetlb.c:hugetlb_vm_op_close() {
	...
	if (reservations) {
		kref_put(&reservations->refs, resv_map_release);
	...
}

If the VMA's reference was dropped concurrently, the reference held by
hugetlb_unmap_file_folio() is the last reference on resv_map->refs. Calling
kref_put(&resv_map->refs, resv_map_release) before
hugetlb_vma_unlock_write(vma) will free resv_map via resv_map_release() ->
kfree(resv_map), and hugetlb_vma_unlock_write(vma) will then call
up_write(&resv_map->rw_sema) on the freed resv_map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028054620.275525-1-eadavis@sina.com?part=1