Re: [PATCH v2] mm/hugetlb: add missing hugetlb_lock in __unmap_hugepage_range()

[Jeongjun Park <aha310510@gmail.com>]

Jeongjun Park <aha310510@gmail.com> wrote:
>
> When restoring a reservation for an anonymous page, we need to check to
> freeing a surplus. However, __unmap_hugepage_range() causes data race
> because it reads h->surplus_huge_pages without the protection of
> hugetlb_lock.
>
> And adjust_reservation is a boolean variable that indicates whether
> reservations for anonymous pages in each folio should be restored.
> Therefore, it should be initialized to false for each round of the loop.
> However, this variable is not initialized to false except when defining
> the current adjust_reservation variable.
>
> This means that once adjust_reservation is set to true even once within
> the loop, reservations for anonymous pages will be restored
> unconditionally in all subsequent rounds, regardless of the folio's state.
>
> To fix this, we need to add the missing hugetlb_lock, unlock the
> page_table_lock earlier so that we don't lock the hugetlb_lock inside the
> page_table_lock lock, and initialize adjust_reservation to false on each
> round within the loop.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: syzbot+417aeb05fd190f3a6da9@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=417aeb05fd190f3a6da9
> Fixes: df7a6d1f6405 ("mm/hugetlb: restore the reservation if needed")

Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>

Sorry, I forgot to add the reviewed-by tag.

> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
> v2: Fix issues with changing the page_table_lock unlock location and initializing adjust_reservation
> - Link to v1: https://lore.kernel.org/all/20250822055857.1142454-1-aha310510@gmail.com/
> ---
>  mm/hugetlb.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 753f99b4c718..eed59cfb5d21 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5851,7 +5851,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>         spinlock_t *ptl;
>         struct hstate *h = hstate_vma(vma);
>         unsigned long sz = huge_page_size(h);
> -       bool adjust_reservation = false;
> +       bool adjust_reservation;
>         unsigned long last_addr_mask;
>         bool force_flush = false;
>
> @@ -5944,6 +5944,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>                                         sz);
>                 hugetlb_count_sub(pages_per_huge_page(h), mm);
>                 hugetlb_remove_rmap(folio);
> +               spin_unlock(ptl);
>
>                 /*
>                  * Restore the reservation for anonymous page, otherwise the
> @@ -5951,14 +5952,16 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
>                  * If there we are freeing a surplus, do not set the restore
>                  * reservation bit.
>                  */
> +               adjust_reservation = false;
> +
> +               spin_lock_irq(&hugetlb_lock);
>                 if (!h->surplus_huge_pages && __vma_private_lock(vma) &&
>                     folio_test_anon(folio)) {
>                         folio_set_hugetlb_restore_reserve(folio);
>                         /* Reservation to be adjusted after the spin lock */
>                         adjust_reservation = true;
>                 }
> -
> -               spin_unlock(ptl);
> +               spin_unlock_irq(&hugetlb_lock);
>
>                 /*
>                  * Adjust the reservation for the region that will have the
> --