Message-ID: <CAGsJ_4x+YaF3bVjMuNv2iJtdoD2-GgGsbJA__cpxb7U5YtK9ig@mail.gmail.com>
Date: Wed, 25 Jun 2025 21:38:21 +1200
From: Barry Song <21cnbao@...il.com>
To: Lance Yang <ioworker0@...il.com>
Cc: david@...hat.com, akpm@...ux-foundation.org, baolin.wang@...ux.alibaba.com, 
	chrisl@...nel.org, kasong@...cent.com, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	linux-riscv@...ts.infradead.org, lorenzo.stoakes@...cle.com, 
	ryan.roberts@....com, v-songbaohua@...o.com, x86@...nel.org, 
	ying.huang@...el.com, zhengtangquan@...o.com
Subject: Re: [PATCH v4 3/4] mm: Support batched unmap for lazyfree large
 folios during reclamation

On Wed, Jun 25, 2025 at 4:27 AM Lance Yang <ioworker0@...il.com> wrote:
>
> On 2025/6/24 23:34, David Hildenbrand wrote:
> > On 24.06.25 17:26, Lance Yang wrote:
> >> On 2025/6/24 20:55, David Hildenbrand wrote:
> >>> On 14.02.25 10:30, Barry Song wrote:
> >>>> From: Barry Song <v-songbaohua@...o.com>
> >> [...]
> >>>> diff --git a/mm/rmap.c b/mm/rmap.c
> >>>> index 89e51a7a9509..8786704bd466 100644
> >>>> --- a/mm/rmap.c
> >>>> +++ b/mm/rmap.c
> >>>> @@ -1781,6 +1781,25 @@ void folio_remove_rmap_pud(struct folio *folio,
> >>>> struct page *page,
> >>>>    #endif
> >>>>    }
> >>>> +/* We support batch unmapping of PTEs for lazyfree large folios */
> >>>> +static inline bool can_batch_unmap_folio_ptes(unsigned long addr,
> >>>> +            struct folio *folio, pte_t *ptep)
> >>>> +{
> >>>> +    const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>> +    int max_nr = folio_nr_pages(folio);
> >>>
> >>> Let's assume we have the first page of a folio mapped at the last page
> >>> table entry in our page table.
> >>
> >> Good point. I'm curious if it is something we've seen in practice ;)
> >
> > I challenge you to write a reproducer :P I assume it might be doable
> > through simple mremap().
> >
> >>
> >>>
> >>> What prevents folio_pte_batch() from reading outside the page table?
> >>
> >> Assuming such a scenario is possible, to prevent any chance of an
> >> out-of-bounds read, how about this change:
> >>
> >> diff --git a/mm/rmap.c b/mm/rmap.c
> >> index fb63d9256f09..9aeae811a38b 100644
> >> --- a/mm/rmap.c
> >> +++ b/mm/rmap.c
> >> @@ -1852,6 +1852,25 @@ static inline bool
> >> can_batch_unmap_folio_ptes(unsigned long addr,
> >>       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>       int max_nr = folio_nr_pages(folio);
> >>       pte_t pte = ptep_get(ptep);
> >> +    unsigned long end_addr;
> >> +
> >> +    /*
> >> +     * To batch unmap, the entire folio's PTEs must be contiguous
> >> +     * and mapped within the same PTE page table, which corresponds to
> >> +     * a single PMD entry. Before calling folio_pte_batch(), which does
> >> +     * not perform boundary checks itself, we must verify that the
> >> +     * address range covered by the folio does not cross a PMD boundary.
> >> +     */
> >> +    end_addr = addr + (max_nr * PAGE_SIZE) - 1;
> >> +
> >> +    /*
> >> +     * A fast way to check for a PMD boundary cross is to align both
> >> +     * the start and end addresses to the PMD boundary and see if they
> >> +     * are different. If they are, the range spans across at least two
> >> +     * different PMD-managed regions.
> >> +     */
> >> +    if ((addr & PMD_MASK) != (end_addr & PMD_MASK))
> >> +        return false;
> >
> > You should not be messing with max_nr = folio_nr_pages(folio) here at
> > all. folio_pte_batch() takes care of that.
> >
> > Also, way too many comments ;)
> >
> > You may only batch within a single VMA and within a single page table.
> >
> > So simply align the addr up to the next PMD, and make sure it does not
> > exceed the vma end.
> >
> > ALIGN and friends can help avoiding excessive comments.
>
> Thanks! How about this updated version based on your suggestion:
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index fb63d9256f09..241d55a92a47 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1847,12 +1847,25 @@ void folio_remove_rmap_pud(struct folio *folio, struct page *page,
>
>  /* We support batch unmapping of PTEs for lazyfree large folios */
>  static inline bool can_batch_unmap_folio_ptes(unsigned long addr,
> -                       struct folio *folio, pte_t *ptep)
> +                                             struct folio *folio, pte_t *ptep,
> +                                             struct vm_area_struct *vma)
>  {
>         const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> +       unsigned long next_pmd, vma_end, end_addr;
>         int max_nr = folio_nr_pages(folio);
>         pte_t pte = ptep_get(ptep);
>
> +       /*
> +        * Limit the batch scan within a single VMA and within a single
> +        * page table.
> +        */
> +       vma_end = vma->vm_end;
> +       next_pmd = ALIGN(addr + 1, PMD_SIZE);
> +       end_addr = addr + (unsigned long)max_nr * PAGE_SIZE;
> +
> +       if (end_addr > min(next_pmd, vma_end))
> +               return false;
> +

I had a similar check in do_swap_page() for both forward and backward
out-of-bounds page tables, but I forgot to add it for this unmap path.

this is do_swap_page():

        if (folio_test_large(folio) && folio_test_swapcache(folio)) {
                int nr = folio_nr_pages(folio);
                unsigned long idx = folio_page_idx(folio, page);
                unsigned long folio_start = address - idx * PAGE_SIZE;
                unsigned long folio_end = folio_start + nr * PAGE_SIZE;

                pte_t *folio_ptep;
                pte_t folio_pte;

                if (unlikely(folio_start < max(address & PMD_MASK,
vma->vm_start)))
                        goto check_folio;
                if (unlikely(folio_end > pmd_addr_end(address, vma->vm_end)))
                        goto check_folio;
       }

So maybe something like folio_end > pmd_addr_end(address, vma->vm_end)?

Thanks
Barry

Powered by blists - more mailing lists