Re: [PATCH RFC v2] mm: use per_vma lock for MADV_DONTNEED

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

Qi - thanks very much for your insightful response, will stash this
somewhere to revisit.

On Fri, Jun 06, 2025 at 11:55:56AM +0800, Qi Zheng wrote:
> Hi Lorenzo,
>
> On 6/5/25 10:04 PM, Lorenzo Stoakes wrote:
> > On Thu, Jun 05, 2025 at 11:23:18AM +0800, Qi Zheng wrote:
> > >
> > >
> > > On 6/5/25 1:50 AM, Lorenzo Stoakes wrote:
> > > > On Wed, Jun 04, 2025 at 02:02:12PM +0800, Qi Zheng wrote:
> > > > > Hi Lorenzo,
> > > > >
> > > > > On 6/3/25 5:54 PM, Lorenzo Stoakes wrote:
> > > > > > On Tue, Jun 03, 2025 at 03:24:28PM +0800, Qi Zheng wrote:
> > > > > > > Hi Jann,
> > > > > > >
> > > > > > > On 5/30/25 10:06 PM, Jann Horn wrote:
> > > > > > > > On Fri, May 30, 2025 at 12:44 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > > > > > > Certain madvise operations, especially MADV_DONTNEED, occur far more
> > > > > > > > > frequently than other madvise options, particularly in native and Java
> > > > > > > > > heaps for dynamic memory management.
> > > > > > > > >
> > > > > > > > > Currently, the mmap_lock is always held during these operations, even when
> > > > > > > > > unnecessary. This causes lock contention and can lead to severe priority
> > > > > > > > > inversion, where low-priority threads—such as Android's HeapTaskDaemon—
> > > > > > > > > hold the lock and block higher-priority threads.
> > > > > > > > >
> > > > > > > > > This patch enables the use of per-VMA locks when the advised range lies
> > > > > > > > > entirely within a single VMA, avoiding the need for full VMA traversal. In
> > > > > > > > > practice, userspace heaps rarely issue MADV_DONTNEED across multiple VMAs.
> > > > > > > > >
> > > > > > > > > Tangquan’s testing shows that over 99.5% of memory reclaimed by Android
> > > > > > > > > benefits from this per-VMA lock optimization. After extended runtime,
> > > > > > > > > 217,735 madvise calls from HeapTaskDaemon used the per-VMA path, while
> > > > > > > > > only 1,231 fell back to mmap_lock.
> > > > > > > > >
> > > > > > > > > To simplify handling, the implementation falls back to the standard
> > > > > > > > > mmap_lock if userfaultfd is enabled on the VMA, avoiding the complexity of
> > > > > > > > > userfaultfd_remove().
> > > > > > > >
> > > > > > > > One important quirk of this is that it can, from what I can see, cause
> > > > > > > > freeing of page tables (through pt_reclaim) without holding the mmap
> > > > > > > > lock at all:
> > > > > > > >
> > > > > > > > do_madvise [behavior=MADV_DONTNEED]
> > > > > > > >       madvise_lock
> > > > > > > >         lock_vma_under_rcu
> > > > > > > >       madvise_do_behavior
> > > > > > > >         madvise_single_locked_vma
> > > > > > > >           madvise_vma_behavior
> > > > > > > >             madvise_dontneed_free
> > > > > > > >               madvise_dontneed_single_vma
> > > > > > > >                 zap_page_range_single_batched [.reclaim_pt = true]
> > > > > > > >                   unmap_single_vma
> > > > > > > >                     unmap_page_range
> > > > > > > >                       zap_p4d_range
> > > > > > > >                         zap_pud_range
> > > > > > > >                           zap_pmd_range
> > > > > > > >                             zap_pte_range
> > > > > > > >                               try_get_and_clear_pmd
> > > > > > > >                               free_pte
> > > > > > > >
> > > > > > > > This clashes with the assumption in walk_page_range_novma() that
> > > > > > > > holding the mmap lock in write mode is sufficient to prevent
> > > > > > > > concurrent page table freeing, so it can probably lead to page table
> > > > > > > > UAF through the ptdump interface (see ptdump_walk_pgd()).
> > > > > > >
> > > > > > > Maybe not? The PTE page is freed via RCU in zap_pte_range(), so in the
> > > > > > > following case:
> > > > > > >
> > > > > > > cpu 0				cpu 1
> > > > > > >
> > > > > > > ptdump_walk_pgd
> > > > > > > --> walk_pte_range
> > > > > > >        --> pte_offset_map (hold RCU read lock)
> > > > > > > 				zap_pte_range
> > > > > > > 				--> free_pte (via RCU)
> > > > > > >            walk_pte_range_inner
> > > > > > >            --> ptdump_pte_entry (the PTE page is not freed at this time)
> > > > > > >
> > > > > > > IIUC, there is no UAF issue here?
> > > > > > >
> > > > > > > If I missed anything please let me know.
> > > >
> > > > Seems to me that we don't need the VMA locks then unless I'm missing
> > > > something? :) Jann?
> > > >
> > > > Would this RCU-lock-acquired-by-pte_offset_map also save us from the
> > > > munmap() downgraded read lock scenario also? Or is the problem there
> > > > intermediate page table teardown I guess?
> > > >
> > >
> > > Right. Currently, page table pages other than PTE pages are not
> > > protected by RCU, so mmap write lock still needed in the munmap path
> > > to wait for all readers of the page table pages to exit the critical
> > > section.
> > >
> > > In other words, once we have achieved that all page table pages are
> > > protected by RCU, we can completely remove the page table pages from
> > > the protection of mmap locks.
> >
> > Interesting - so on reclaim/migrate we are just clearing PTE entries with
> > the rmap lock right? Would this lead to a future where we could also tear
> > down page tables there?
> >
> > Another point to remember is that when we are clearing down higher level
> > page tables in the general case, the logic assumes nothing else can touch
> > anything... we hold both rmap lock AND mmap/vma locks at this point.
> >
> > But I guess if we're RCU-safe, we're same even from rmap right?
>
> Yeah, and we have already done something similar. For more details,
> please refer to retract_page_tables(). It only holds i_mmap_rwsem read
> lock and then calls pte_free_defer() to free the PTE page through RCU.

Yeah, but that i_mmap_rwsem is important :) as it protects against other
rmap users.

Interesting that we only do this for shmem case not anon...

>
> For migrate case, the pte entry will store a migrate entry, right? And a
> new physical page will be installed soon through a page fault, so I
> don't think it is necessary to free the corresponding PTE page.

Yeah.

>
> For reclaim case, there is a problem that only PTE entries that mapped
> to a physical page are operated each time. If we want to free the entire
> PTE page, we need to check the adjacent PTE entries. Maybe MGLRU can
> help with this. I remember that MGLRU has an optimization that will check
> the adjacent PTE entries.

Yeah indeed, we'd need to take the 'very simple' reclaim code and have it
do _even more_ in this case :P

>
> >
> > >
> > > Here are some of my previous thoughts:
> > >
> > > ```
> > > Another plan
> > > ============
> > >
> > > Currently, page table modification are protected by page table locks
> > > (page_table_lock or split pmd/pte lock), but the life cycle of page
> > > table pages are protected by mmap_lock (and vma lock). For more details,
> > > please refer to the latest added Documentation/mm/process_addrs.rst file.
> > >
> > > Currently we try to free the PTE pages through RCU when
> > > CONFIG_PT_RECLAIM is turned on. In this case, we will no longer
> > > need to hold mmap_lock for the read/write op on the PTE pages.
> > >
> > > So maybe we can remove the page table from the protection of the mmap
> > > lock (which is too big), like this:
> > >
> > > 1. free all levels of page table pages by RCU, not just PTE pages, but
> > >     also pmd, pud, etc.
> > > 2. similar to pte_offset_map/pte_unmap, add
> > >     [pmd|pud]_offset_map/[pmd|pud]_unmap, and make them all contain
> > >     rcu_read_lock/rcu_read_unlcok, and make them accept failure.
> > >
> > > In this way, we no longer need the mmap lock. For readers, such as page
> > > table wallers, we are already in the critical section of RCU. For
> > > writers, we only need to hold the page table lock.
> > >
> > > But there is a difficulty here, that is, the RCU critical section is not
> > > allowed to sleep, but it is possible to sleep in the callback function
> > > of .pmd_entry, such as mmu_notifier_invalidate_range_start().
> > >
> > > Use SRCU instead? Or use RCU + refcount method? Not sure. But I think
> > > it's an interesting thing to try.
> >
> > Thanks for the information, RCU freeing of page tables is something of a
>
> RCU-freeing is relatively simple, tlb_remove_table() can be easily
> changed to free all levels of page table pages through RCU. The more
> difficult is to protect the page table pages above PTE level through RCU
> lock.
>
> > long-term TODO discussed back and forth :) might take a look myself if
> > somebody else hasn't grabbed when I have a second...
>
> This is awesome, I'm stuck with some other stuff at the moment, I'll
> also take a look at it later when I have time.

Yeah, I guess good to ping on-list if/when one of us/somebody else takes a
look to synchronise :)

>
> >
> > Is it _only_ the mmu notifier sleeping in this scenario? Or are there other
> > examples?
>
> I'm not sure, need some investigation.
>
> >
> > We could in theory always add another callback .pmd_entry_sleep or
> > something for this one case and document the requirement...
>
> Maybe, but the SRCU critical section cannot prevent the PTE page from
> being freed via RCU. :(

Idea is we'd fall back to non-RCU in this case and take locks... but then
ugh we'd race everything RCU and no it's all or nothing isn't it?

Overall - I will stash this response somewhere and come back to it if
somebody else doesn't in the meantime :)

>
> Thanks!
>
> >
> > > ```
> > >
> > > Thanks!
> > >
> > >
> >
> > Cheers, Lorenzo
>