| 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: <d5fd1a91-0b32-4eb8-aa4c-2f4e0e1ef102@lucifer.local>
Date: Fri, 20 Jun 2025 15:58:55 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>, Zi Yan <ziy@...dia.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
Dev Jain <dev.jain@....com>, Barry Song <baohua@...nel.org>,
Jann Horn <jannh@...gle.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Lance Yang <ioworker0@...il.com>,
SeongJae Park <sj@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH 4/5] mm/madvise: thread all madvise state through
madv_behavior
On Fri, Jun 20, 2025 at 04:32:48PM +0200, Vlastimil Babka wrote:
> On 6/19/25 22:26, Lorenzo Stoakes wrote:
> > Doing so means we can get rid of all the weird struct vm_area_struct **prev
> > stuff, everything becomes consistent and in future if we want to make
> > change to behaviour there's a single place where all relevant state is
> > stored.
> >
> > This also allows us to update try_vma_read_lock() to be a little more
> > succinct and set up state for us, as well as cleaning up
> > madvise_update_vma().
> >
> > We also update the debug assertion prior to madvise_update_vma() to assert
> > that this is a write operation as correctly pointed out by Barry in the
> > relevant thread.
> >
> > We can't reasonably update the madvise functions that live outside of
> > mm/madvise.c so we leave those as-is.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
Thanks!
>
> The prev manipulation is indeed confusing, looking forward to the next patch...
Yeah this was the whole motivation for the series, ultimately...
>
> Nits:
>
> > ---
> > mm/madvise.c | 283 ++++++++++++++++++++++++++-------------------------
> > 1 file changed, 146 insertions(+), 137 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 6faa38b92111..86fe04aa7c88 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -74,6 +74,8 @@ struct madvise_behavior {
> > * traversing multiple VMAs, this is updated for each.
> > */
> > struct madvise_behavior_range range;
> > + /* The VMA and VMA preceding it (if applicable) currently targeted. */
> > + struct vm_area_struct *prev, *vma;
>
> Would also do separate lines here.
Ack will do.
>
> > -static long madvise_dontneed_free(struct vm_area_struct *vma,
> > - struct vm_area_struct **prev,
> > - unsigned long start, unsigned long end,
> > - struct madvise_behavior *madv_behavior)
> > +static long madvise_dontneed_free(struct madvise_behavior *madv_behavior)
> > {
> > + struct mm_struct *mm = madv_behavior->mm;
> > + struct madvise_behavior_range *range = &madv_behavior->range;
> > int behavior = madv_behavior->behavior;
> > - struct mm_struct *mm = vma->vm_mm;
> >
> > - *prev = vma;
> > - if (!madvise_dontneed_free_valid_vma(vma, start, &end, behavior))
> > + madv_behavior->prev = madv_behavior->vma;
> > + if (!madvise_dontneed_free_valid_vma(madv_behavior))
> > return -EINVAL;
> >
> > - if (start == end)
> > + if (range->start == range->end)
> > return 0;
> >
> > - if (!userfaultfd_remove(vma, start, end)) {
> > - *prev = NULL; /* mmap_lock has been dropped, prev is stale */
> > + if (!userfaultfd_remove(madv_behavior->vma, range->start, range->end)) {
> > + struct vm_area_struct *vma;
> > +
> > + madv_behavior->prev = NULL; /* mmap_lock has been dropped, prev is stale */
> >
> > mmap_read_lock(mm);
> > - vma = vma_lookup(mm, start);
> > + madv_behavior->vma = vma = vma_lookup(mm, range->start);
>
> This replaces vma in madv_behavior...
Yeah note that the lock is dropped here also :) so the previous VMA is
completely invalidated, so this is valid.
>
> > @@ -1617,23 +1625,19 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> > struct mm_struct *mm = madv_behavior->mm;
> > struct madvise_behavior_range *range = &madv_behavior->range;
> > unsigned long end = range->end;
> > - struct vm_area_struct *vma;
> > - struct vm_area_struct *prev;
> > int unmapped_error = 0;
> > int error;
> > + struct vm_area_struct *vma;
> >
> > /*
> > * If VMA read lock is supported, apply madvise to a single VMA
> > * tentatively, avoiding walking VMAs.
> > */
> > - if (madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> > - vma = try_vma_read_lock(madv_behavior);
> > - if (vma) {
> > - prev = vma;
> > - error = madvise_vma_behavior(vma, &prev, madv_behavior);
> > - vma_end_read(vma);
> > - return error;
> > - }
> > + if (madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK &&
> > + try_vma_read_lock(madv_behavior)) {
> > + error = madvise_vma_behavior(madv_behavior);
> > + vma_end_read(madv_behavior->vma);
> > + return error;
>
> And here we could potentially do vma_end_read() on the replaced vma. And
> it's exactly cases using madvise_dontneed_free() that use
> MADVISE_VMA_READ_LOCK mode. But it's not an issue as try_vma_read_lock()
> will fail with uffd and that vma replacement scenario is tied to
> userfaultfd_remove(). It's just quite tricky, hm...
Yeah this kind of thing is explicitly why Barry excluded uffd replace from the
per-VMA lock I think.
So we absolutely can't use the per-VMA lock in conjunction with
uffd_remove(). I'll add a debug assert in madvise_dontneed_free() to make this
clearer.
Will add in madvise_remove() too as that does the same thing there, just to make
sure we have assert coverage.
>
> > }
> >
> > /*
> > @@ -1641,11 +1645,13 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> > * ranges, just ignore them, but return -ENOMEM at the end.
> > * - different from the way of handling in mlock etc.
> > */
> > - vma = find_vma_prev(mm, range->start, &prev);
> > + vma = find_vma_prev(mm, range->start, &madv_behavior->prev);
> > if (vma && range->start > vma->vm_start)
> > - prev = vma;
> > + madv_behavior->prev = vma;
> >
> > for (;;) {
> > + struct vm_area_struct *prev;
> > +
> > /* Still start < end. */
> > if (!vma)
> > return -ENOMEM;
> > @@ -1662,13 +1668,16 @@ int madvise_walk_vmas(struct madvise_behavior *madv_behavior)
> > range->end = min(vma->vm_end, end);
> >
> > /* Here vma->vm_start <= range->start < range->end <= (end|vma->vm_end). */
> > - error = madvise_vma_behavior(vma, &prev, madv_behavior);
> > + madv_behavior->vma = vma;
> > + error = madvise_vma_behavior(madv_behavior);
> > if (error)
> > return error;
> > + prev = madv_behavior->prev;
> > +
> > range->start = range->end;
> > if (prev && range->start < prev->vm_end)
> > range->start = prev->vm_end;
> > - if (range->start >= range->end)
> > + if (range->start >= end)
> > break;
> > if (prev)
> > vma = find_vma(mm, prev->vm_end);
>
Powered by blists - more mailing lists