| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a495779a-5351-48bb-93e4-6ccf52631796@lucifer.local>
Date: Wed, 11 Jun 2025 11:07:56 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Barry Song <21cnbao@...il.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Barry Song <v-songbaohua@...o.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
David Hildenbrand <david@...hat.com>, Vlastimil Babka <vbabka@...e.cz>,
Jann Horn <jannh@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>,
Lokesh Gidra <lokeshgidra@...gle.com>, Mike Rapoport <rppt@...nel.org>,
Michal Hocko <mhocko@...e.com>,
Tangquan Zheng <zhengtangquan@...o.com>,
Qi Zheng <zhengqi.arch@...edance.com>
Subject: Re: [PATCH RFC] mm: madvise: use per_vma lock for MADV_FREE
On Wed, Jun 11, 2025 at 10:03:05PM +1200, Barry Song wrote:
> On Wed, Jun 11, 2025 at 6:52 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > On Tue, Jun 10, 2025 at 05:59:20PM +1200, Barry Song wrote:
> > > From: Barry Song <v-songbaohua@...o.com>
> > >
> > > MADV_FREE is another option, besides MADV_DONTNEED, for dynamic memory
> > > freeing in user-space native or Java heap memory management. For example,
> > > jemalloc can be configured to use MADV_FREE, and recent versions of the
> > > Android Java heap have also increasingly adopted MADV_FREE. Supporting
> > > per-VMA locking for MADV_FREE thus appears increasingly necessary.
> > >
> > > We have replaced walk_page_range() with walk_page_range_vma(). Along with
> > > the proposed madvise_lock_mode by Lorenzo, the necessary infrastructure is
> > > now in place to begin exploring per-VMA locking support for MADV_FREE and
> > > potentially other madvise using walk_page_range_vma().
> >
> > Thanks :)
> >
> > >
> > > This patch adds support for the PGWALK_VMA_RDLOCK walk_lock mode in
> > > walk_page_range_vma(), and leverages madvise_lock_mode from
> > > madv_behavior to select the appropriate walk_lock—either mmap_lock or
> > > per-VMA lock—based on the context.
> > >
> > > To ensure thread safety, madvise_free_walk_ops is now defined as a stack
> > > variable instead of a global constant.
> >
> > A nit but I'd add 'because we now dynamically update the walk_ops->walk_lock
> > field we must make sure this is thread safe' or something like this to clarify
> > the need for this
>
> Sure.
Thanks!
>
> >
> > Did we not have to worry about this before I guess because the mmap lock would
> > exclude other threads?
>
> Probably not. It was a constant, and no one needed to modify it
> before, no matter how many threads called MADV_FREE.
Yeah of course, I wrote this before I went and looked more carefully and comment
preceding this and... anyway yeah :)
>
> >
> > An aside, but I wonder if we have this implicit assumption elsewhere that VMA
> > locks defeat... hm :)
This is more the concern, but not to do with your series but more a general
thing 'in how many places do we make implicit assumptions about things like
thread concurrency due to locking we presume we have?'
Hopefully this isn't common :>)
> >
> > >
> > > Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> > > Cc: David Hildenbrand <david@...hat.com>
> > > Cc: Vlastimil Babka <vbabka@...e.cz>
> > > Cc: Jann Horn <jannh@...gle.com>
> > > Cc: Suren Baghdasaryan <surenb@...gle.com>
> > > Cc: Lokesh Gidra <lokeshgidra@...gle.com>
> > > Cc: Mike Rapoport <rppt@...nel.org>
> > > Cc: Michal Hocko <mhocko@...e.com>
> > > Cc: Tangquan Zheng <zhengtangquan@...o.com>
> > > Cc: Qi Zheng <zhengqi.arch@...edance.com>
> > > Signed-off-by: Barry Song <v-songbaohua@...o.com>
> >
> > Looks good to me, kinda neat how the previous work for the MADV_DONTNEED under
> > VMA lock stuff made this pretty straightforward :)
> >
> > So:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Thanks!
No problem! Thanks for the patch, this is a nice step forward.
>
> >
> > > ---
> > > include/linux/pagewalk.h | 2 ++
> > > mm/madvise.c | 20 ++++++++++++++------
> > > mm/pagewalk.c | 6 ++++++
> > > 3 files changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
> > > index 9700a29f8afb..a4afa64ef0ab 100644
> > > --- a/include/linux/pagewalk.h
> > > +++ b/include/linux/pagewalk.h
> > > @@ -14,6 +14,8 @@ enum page_walk_lock {
> > > PGWALK_WRLOCK = 1,
> > > /* vma is expected to be already write-locked during the walk */
> > > PGWALK_WRLOCK_VERIFY = 2,
> > > + /* vma is expected to be already read-locked during the walk */
> > > + PGWALK_VMA_RDLOCK_VERIFY = 3,
> > > };
> > >
> > > /**
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 381eedde8f6d..23d58eb31c8f 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -775,10 +775,14 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> > > return 0;
> > > }
> > >
> > > -static const struct mm_walk_ops madvise_free_walk_ops = {
> > > - .pmd_entry = madvise_free_pte_range,
> > > - .walk_lock = PGWALK_RDLOCK,
> > > -};
> > > +static inline enum page_walk_lock get_walk_lock(enum madvise_lock_mode mode)
> > > +{
> > > + /* Other modes don't require fixing up the walk_lock. */
> > > + VM_WARN_ON_ONCE(mode != MADVISE_VMA_READ_LOCK &&
> > > + mode != MADVISE_MMAP_READ_LOCK);
> >
> > I find this a bit hard to parse...
> >
> > > + return mode == MADVISE_VMA_READ_LOCK ?
> > > + PGWALK_VMA_RDLOCK_VERIFY : PGWALK_RDLOCK;
> >
> > ...might be better as something like:
> >
> > switch (mode) {
> > case MADVISE_VMA_READ_LOCK:
> > return PGWALK_VMA_RDLOCK_VERIFY;
> > case MADVISE_MMAP_READ_LOCK:
> > return PGWALK_RDLOCK;
> > default:
> > /* Invalid. */
> > WARN_ON_ONCE(1);
> > return PGWALK_RDLOCK;
> > }
>
> I actually tried both before sending and, for some reason, preferred
> the one I sent. But I'm totally happy to go with whichever approach
> you prefer:-)
It's not a big deal, obviously I'd prefer this version but not a blocker!
>
> >
> > > +}
> > >
> > > static int madvise_free_single_vma(struct madvise_behavior *madv_behavior,
> > > struct vm_area_struct *vma,
> > > @@ -787,6 +791,9 @@ static int madvise_free_single_vma(struct madvise_behavior *madv_behavior,
> > > struct mm_struct *mm = vma->vm_mm;
> > > struct mmu_notifier_range range;
> > > struct mmu_gather *tlb = madv_behavior->tlb;
> > > + struct mm_walk_ops walk_ops = {
> > > + .pmd_entry = madvise_free_pte_range,
> > > + };
> > >
> > > /* MADV_FREE works for only anon vma at the moment */
> > > if (!vma_is_anonymous(vma))
> > > @@ -806,8 +813,9 @@ static int madvise_free_single_vma(struct madvise_behavior *madv_behavior,
> > >
> > > mmu_notifier_invalidate_range_start(&range);
> > > tlb_start_vma(tlb, vma);
> > > + walk_ops.walk_lock = get_walk_lock(madv_behavior->lock_mode);
> > > walk_page_range_vma(vma, range.start, range.end,
> > > - &madvise_free_walk_ops, tlb);
> > > + &walk_ops, tlb);
> > > tlb_end_vma(tlb, vma);
> > > mmu_notifier_invalidate_range_end(&range);
> > > return 0;
> > > @@ -1653,7 +1661,6 @@ static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavi
> > > case MADV_WILLNEED:
> > > case MADV_COLD:
> > > case MADV_PAGEOUT:
> > > - case MADV_FREE:
> > > case MADV_POPULATE_READ:
> > > case MADV_POPULATE_WRITE:
> > > case MADV_COLLAPSE:
> > > @@ -1662,6 +1669,7 @@ static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavi
> > > return MADVISE_MMAP_READ_LOCK;
> > > case MADV_DONTNEED:
> > > case MADV_DONTNEED_LOCKED:
> > > + case MADV_FREE:
> > > return MADVISE_VMA_READ_LOCK;
> > > default:
> > > return MADVISE_MMAP_WRITE_LOCK;
> > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> > > index e478777c86e1..c984aacc5552 100644
> > > --- a/mm/pagewalk.c
> > > +++ b/mm/pagewalk.c
> > > @@ -420,6 +420,9 @@ static int __walk_page_range(unsigned long start, unsigned long end,
> > > static inline void process_mm_walk_lock(struct mm_struct *mm,
> > > enum page_walk_lock walk_lock)
> > > {
> > > + if (walk_lock == PGWALK_VMA_RDLOCK_VERIFY)
> > > + return;
> > > +
> > > if (walk_lock == PGWALK_RDLOCK)
> > > mmap_assert_locked(mm);
> > > else
> > > @@ -437,6 +440,9 @@ static inline void process_vma_walk_lock(struct vm_area_struct *vma,
> > > case PGWALK_WRLOCK_VERIFY:
> > > vma_assert_write_locked(vma);
> > > break;
> > > + case PGWALK_VMA_RDLOCK_VERIFY:
> > > + vma_assert_locked(vma);
> > > + break;
> > > case PGWALK_RDLOCK:
> > > /* PGWALK_RDLOCK is handled by process_mm_walk_lock */
> > > break;
> > > --
> > > 2.39.3 (Apple Git-146)
> > >
>
> Thanks
> Barry
Cheers, Lorenzo
Powered by blists - more mailing lists