linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Lorenzo Stoakes (Oracle)" <ljs@kernel.org>
To: "David Hildenbrand (Arm)" <david@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	 "Liam R . Howlett" <Liam.Howlett@oracle.com>,
	Vlastimil Babka <vbabka@kernel.org>,
	 Jann Horn <jannh@google.com>, Pedro Falcato <pfalcato@suse.de>,
	Jeff Xu <jeffxu@chromium.org>,
	 linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	 antonius <bluedragonsec2023@gmail.com>
Subject: Re: [PATCH mm-hotfixes] mm/mseal: update VMA end correctly on merge
Date: Fri, 27 Mar 2026 17:23:34 +0000	[thread overview]
Message-ID: <5739bab8-d75f-404d-a3c8-a67f91e072a5@lucifer.local> (raw)
In-Reply-To: <d82c61ed-32ba-47ed-bb85-9facdee7c26f@kernel.org>

On Fri, Mar 27, 2026 at 05:57:06PM +0100, David Hildenbrand (Arm) wrote:
> On 3/27/26 10:06, Lorenzo Stoakes (Oracle) wrote:
> > Previously we stored the end of the current VMA in curr_end, and then upon
> > iterating to the next VMA updated curr_start to curr_end to advance to the
> > next VMA.
> >
> > However, this doesn't take into account the fact that a VMA might be
> > updated due to a merge by vma_modify_flags(), which can result in curr_end
> > being stale and thus, upon setting curr_start to curr_end, ending up with
> > an incorrect curr_start on the next iteration.
> >
> > Resolve the issue by setting curr_end to vma->vm_end unconditionally to
> > ensure this value remains updated should this occur.
> >
> > Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
> > Fixes: 6c2da14ae1e0 ("mm/mseal: rework mseal apply logic")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Antonius <antonius@bluedragonsec.com>
> > Closes: https://lore.kernel.org/linux-mm/CAK8a0jyHXqBpt8Xe8v9SNDbnRiwz7OthA8SKY=NLRY7smPEP3Q@mail.gmail.com/
> > ---
> >  mm/mseal.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index 316b5e1dec78..2d72a15d8ea1 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -66,7 +66,7 @@ static int mseal_apply(struct mm_struct *mm,
> >  		prev = vma;
> >
> >  	for_each_vma_range(vmi, vma, end) {
> > -		const unsigned long curr_end = MIN(vma->vm_end, end);
> > +		unsigned long curr_end = MIN(vma->vm_end, end);
> >
> >  		if (!(vma->vm_flags & VM_SEALED)) {
> >  			vm_flags_t vm_flags = vma->vm_flags | VM_SEALED;
> > @@ -76,6 +76,7 @@ static int mseal_apply(struct mm_struct *mm,
> >  			if (IS_ERR(vma))
> >  				return PTR_ERR(vma);
> >  			vm_flags_set(vma, VM_SEALED);
> > +			curr_end = vma->vm_end; /* Merge may have updated. */
> >  		}
>
>
> I was a bit confused why curr_start is allowed to not start within the VMA,
> but before it. Then I recalled that range_contains_unmapped() checks for no holes.
>
>
> Would the following also sort out the problem and even simplify the code?
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index 603df53ad267..e2093ae3d25c 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -56,7 +56,6 @@ static int mseal_apply(struct mm_struct *mm,
>                 unsigned long start, unsigned long end)
>  {
>         struct vm_area_struct *vma, *prev;
> -       unsigned long curr_start = start;
>         VMA_ITERATOR(vmi, mm, start);
>
>         /* We know there are no gaps so this will be non-NULL. */
> @@ -66,6 +65,7 @@ static int mseal_apply(struct mm_struct *mm,
>                 prev = vma;
>
>         for_each_vma_range(vmi, vma, end) {
> +               const unsigned long curr_start = MAX(vma->vm_start, start);

Yeah that's nice :)


>                 const unsigned long curr_end = MIN(vma->vm_end, end);
>
>                 if (!vma_test(vma, VMA_SEALED_BIT)) {
> @@ -82,7 +82,6 @@ static int mseal_apply(struct mm_struct *mm,
>                 }
>
>                 prev = vma;
> -               curr_start = curr_end;
>         }
>
>         return 0;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>
>
> I might be wrong about that, I've been staring at the screen for too long today.

No this is better, I've mulled this over, this is much better :)

Sorry to be a pain Andrew - let me respin this right now.

I will retain tags as this is functionally equivalent, I have checked it
locally, confirmed repro solved, checked against AI review also.

>
>
> --
> Cheers,
>
> David

Cheers, Lorenzo


      reply	other threads:[~2026-03-27 17:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-27  9:06 Lorenzo Stoakes (Oracle)
2026-03-27  9:15 ` Pedro Falcato
2026-03-27  9:16 ` Lorenzo Stoakes (Oracle)
2026-03-27 13:22 ` Vlastimil Babka (SUSE)
2026-03-27 15:24 ` Andrew Morton
2026-03-27 15:52   ` Lorenzo Stoakes (Oracle)
2026-03-27 16:57 ` David Hildenbrand (Arm)
2026-03-27 17:23   ` Lorenzo Stoakes (Oracle) [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5739bab8-d75f-404d-a3c8-a67f91e072a5@lucifer.local \
    --to=ljs@kernel.org \
    --cc=Liam.Howlett@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=bluedragonsec2023@gmail.com \
    --cc=david@kernel.org \
    --cc=jannh@google.com \
    --cc=jeffxu@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=pfalcato@suse.de \
    --cc=vbabka@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox