| 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: <4c51bd5c-59cc-45b8-8d17-1a2b7b99cb55@lucifer.local>
Date: Wed, 16 Jul 2025 15:53:45 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>, Vlastimil Babka <vbabka@...e.cz>,
Jann Horn <jannh@...gle.com>, Pedro Falcato <pfalcato@...e.de>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Jeff Xu <jeffxu@...omium.org>
Subject: Re: [PATCH v2 5/5] mm/mseal: rework mseal apply logic
Sorry I actually replied to this yestreday, but then closed down emacs and
turned my PC off before I had sent it mistakenly thinking I had :P doh!
On Tue, Jul 15, 2025 at 12:09:10PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@...cle.com> [250715 09:37]:
> > The logic can be simplified - firstly by renaming the inconsistently named
> > apply_mm_seal() to mseal_apply().
> >
> > We then wrap mseal_fixup() into the main loop as the logic is simple enough
> > to not require it, equally it isn't a hugely pleasant pattern in mprotect()
> > etc. so it's not something we want to perpetuate.
> >
> > We remove some redundant comments, and then avoid the entirely unnecessary
> > and slightly bizarre invocation of vma_iter_end() on each loop - really
> > what we want, given we have asserted there are no gaps in the range - is to
> > handle start, end being offset into a VMAs. This is easily handled with
> > MIN()/MAX().
>
> The vma_iter_end() was to detect a merge with the next vma and use the
> new end, since the code you are replacing didn't update the vma pointer.
Ah interesting, good that we now no longer need to do this!
>
> >
> > There's no need to have an 'out' label block since on vma_modify_flags()
> > error we abort anyway.
>
> Ah, in the mseal_fixup()/mseal_apply(). The out label still exists (and
> appears in the below patch) in do_mseal().
Yeah indeed this is what I was referring to :)
>
>
> >
> > And by refactoring like this we avoid the rather horrid 'pass pointer to
> > prev around' pattern used in mprotect() et al.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > Reviewed-by: Pedro Falcato <pfalcato@...e.de>
> > ---
> > mm/mseal.c | 69 +++++++++++++++++-------------------------------------
> > 1 file changed, 22 insertions(+), 47 deletions(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index 794d1043a706..e0fe37623632 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -15,60 +15,35 @@
> > #include <linux/sched.h>
> > #include "internal.h"
> >
> > -static int mseal_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > - struct vm_area_struct **prev, unsigned long start,
> > - unsigned long end, vm_flags_t newflags)
> > -{
> > - int ret = 0;
> > - vm_flags_t oldflags = vma->vm_flags;
> > -
> > - if (newflags == oldflags)
> > - goto out;
> > -
> > - vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> > - if (IS_ERR(vma)) {
> > - ret = PTR_ERR(vma);
> > - goto out;
> > - }
> > -
> > - vm_flags_set(vma, VM_SEALED);
> > -out:
> > - *prev = vma;
> > - return ret;
> > -}
> > -
> > -/*
> > - * Apply sealing.
> > - */
> > -static int apply_mm_seal(unsigned long start, unsigned long end)
> > +static int mseal_apply(struct mm_struct *mm,
> > + unsigned long start, unsigned long end)
> > {
> > - unsigned long nstart;
> > struct vm_area_struct *vma, *prev;
> > - VMA_ITERATOR(vmi, current->mm, start);
> > + VMA_ITERATOR(vmi, mm, start);
> >
> > + /* We know there are no gaps so this will be non-NULL. */
> > vma = vma_iter_load(&vmi);
> > - /*
> > - * Note: check_mm_seal should already checked ENOMEM case.
> > - * so vma should not be null, same for the other ENOMEM cases.
> > - */
> > prev = vma_prev(&vmi);
> > if (start > vma->vm_start)
> > prev = vma;
> >
> > - nstart = start;
> > for_each_vma_range(vmi, vma, end) {
> > - int error;
> > - unsigned long tmp;
> > - vm_flags_t newflags;
> > -
> > - newflags = vma->vm_flags | VM_SEALED;
> > - tmp = vma->vm_end;
> > - if (tmp > end)
> > - tmp = end;
> > - error = mseal_fixup(&vmi, vma, &prev, nstart, tmp, newflags);
> > - if (error)
> > - return error;
> > - nstart = vma_iter_end(&vmi);
> > + unsigned long curr_start, curr_end;
> > +
> > + if (vma->vm_flags & VM_SEALED) {
> > + prev = vma;
> > + continue;
> > + }
> > + curr_start = MAX(start, vma->vm_start);
> > + curr_end = MIN(vma->vm_end, end);
>
> If you assigned curr_start outside the loop, you could just set it to
> vma->vm_end after you change the flags, this would reduce instructions
> in the loop. But this is hardly a performance critical path.
>
> And we probably hardly ever execute this loop more than once.
True, could do a curr_start = curr_end too which is sort of nicely
self-documenting as well.
I need to respin anyway based on other feedback so will do this!
>
> > +
> > + vma = vma_modify_flags(&vmi, prev, vma, curr_start, curr_end,
> > + vma->vm_flags | VM_SEALED);
> > + if (IS_ERR(vma))
> > + return PTR_ERR(vma);
> > + vm_flags_set(vma, VM_SEALED);
> > +
> > + prev = vma;
> > }
> >
> > return 0;
> > @@ -185,10 +160,10 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
> > * reaching the max supported VMAs, however, those cases shall
> > * be rare.
> > */
> > - ret = apply_mm_seal(start, end);
> > + ret = mseal_apply(mm, start, end);
> >
> > out:
> > - mmap_write_unlock(current->mm);
> > + mmap_write_unlock(mm);
>
> It is nice to see the local variable being used.
:) yeah good to refactor that.
>
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>
Thanks!
>
> > return ret;
> > }
> >
> > --
> > 2.50.1
Powered by blists - more mailing lists