linux-kernel - Re: [PATCH v4 4/7] mm: replace vma->vm_flags direct modifications with modifier calls

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y9pZtC+IEjVQO6fh@hyeyoo>
Date:   Wed, 1 Feb 2023 21:23:16 +0900
From:   Hyeonggon Yoo <42.hyeyoo@...il.com>
To:     Suren Baghdasaryan <surenb@...gle.com>
Cc:     akpm@...ux-foundation.org, michel@...pinasse.org,
        jglisse@...gle.com, mhocko@...e.com, vbabka@...e.cz,
        hannes@...xchg.org, mgorman@...hsingularity.net, dave@...olabs.net,
        willy@...radead.org, liam.howlett@...cle.com, peterz@...radead.org,
        ldufour@...ux.ibm.com, paulmck@...nel.org, mingo@...hat.com,
        will@...nel.org, luto@...nel.org, songliubraving@...com,
        peterx@...hat.com, david@...hat.com, dhowells@...hat.com,
        hughd@...gle.com, bigeasy@...utronix.de, kent.overstreet@...ux.dev,
        punit.agrawal@...edance.com, lstoakes@...il.com,
        peterjung1337@...il.com, rientjes@...gle.com,
        axelrasmussen@...gle.com, joelaf@...gle.com, minchan@...gle.com,
        rppt@...nel.org, jannh@...gle.com, shakeelb@...gle.com,
        tatashin@...gle.com, edumazet@...gle.com, gthelen@...gle.com,
        gurua@...gle.com, arjunroy@...gle.com, soheil@...gle.com,
        leewalsh@...gle.com, posk@...gle.com, linux-mm@...ck.org,
        linux-arm-kernel@...ts.infradead.org,
        linuxppc-dev@...ts.ozlabs.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, kernel-team@...roid.com,
        Sebastian Reichel <sebastian.reichel@...labora.com>
Subject: Re: [PATCH v4 4/7] mm: replace vma->vm_flags direct modifications
 with modifier calls

On Tue, Jan 31, 2023 at 10:54:22AM -0800, Suren Baghdasaryan wrote:
> On Tue, Jan 31, 2023 at 12:32 AM Hyeonggon Yoo <42.hyeyoo@...il.com> wrote:
> >
> > On Thu, Jan 26, 2023 at 11:37:49AM -0800, Suren Baghdasaryan wrote:
> > > Replace direct modifications to vma->vm_flags with calls to modifier
> > > functions to be able to track flag changes and to keep vma locking
> > > correctness.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > Acked-by: Michal Hocko <mhocko@...e.com>
> > > Acked-by: Mel Gorman <mgorman@...hsingularity.net>
> > > Acked-by: Mike Rapoport (IBM) <rppt@...nel.org>
> > > Acked-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> > > ---
> > >  arch/arm/kernel/process.c                          |  2 +-
> > >  120 files changed, 188 insertions(+), 199 deletions(-)
> > >
> >
> > Hello Suren,
> 
> Hi Hyeonggon,
> 
> >
> > [...]
> >
> > Whoa, it's so long.
> > Mostly looks fine but two things I'm not sure about:
> >
> > > diff --git a/drivers/misc/open-dice.c b/drivers/misc/open-dice.c
> > > index 9dda47b3fd70..7be4e6c9f120 100644
> > > --- a/drivers/misc/open-dice.c
> > > +++ b/drivers/misc/open-dice.c
> > > @@ -95,12 +95,12 @@ static int open_dice_mmap(struct file *filp, struct vm_area_struct *vma)
> > >               if (vma->vm_flags & VM_WRITE)
> > >                       return -EPERM;
> > >               /* Ensure userspace cannot acquire VM_WRITE later. */
> > > -             vma->vm_flags &= ~VM_MAYWRITE;
> > > +             vm_flags_clear(vma, VM_MAYSHARE);
> > >       }
> >
> > I think it should be:
> >         s/VM_MAYSHARE/VM_MAYWRITE/
> 
> Good eye! Yes, this is definitely a bug. Will post a next version with this fix.
> 
> >
> > > diff --git a/mm/mlock.c b/mm/mlock.c
> > > index 5c4fff93cd6b..ed49459e343e 100644
> > > --- a/mm/mlock.c
> > > +++ b/mm/mlock.c
> > > @@ -380,7 +380,7 @@ static void mlock_vma_pages_range(struct vm_area_struct *vma,
> > >        */
> > >       if (newflags & VM_LOCKED)
> > >               newflags |= VM_IO;
> > > -     WRITE_ONCE(vma->vm_flags, newflags);
> > > +     vm_flags_reset(vma, newflags);
> > >
> > >       lru_add_drain();
> > >       walk_page_range(vma->vm_mm, start, end, &mlock_walk_ops, NULL);
> > > @@ -388,7 +388,7 @@ static void mlock_vma_pages_range(struct vm_area_struct *vma,
> > >
> > >       if (newflags & VM_IO) {
> > >               newflags &= ~VM_IO;
> > > -             WRITE_ONCE(vma->vm_flags, newflags);
> > > +             vm_flags_reset(vma, newflags);
> > >       }
> > >  }
> >
> > wondering the if the comment above is still true?
> >
> >         /*
> >          * There is a slight chance that concurrent page migration,
> >          * or page reclaim finding a page of this now-VM_LOCKED vma,
> >          * will call mlock_vma_folio() and raise page's mlock_count:
> >          * double counting, leaving the page unevictable indefinitely.
> >          * Communicate this danger to mlock_vma_folio() with VM_IO,
> >          * which is a VM_SPECIAL flag not allowed on VM_LOCKED vmas.
> >          * mmap_lock is held in write mode here, so this weird
> >          * combination should not be visible to other mmap_lock users;
> >          * but WRITE_ONCE so rmap walkers must see VM_IO if VM_LOCKED.
> >          */
> >
> > does ACCESS_PRIVATE() still guarentee that compiler cannot mysteriously
> > optimize writes like WRITE_ONCE()?
> 
> I don't see ACCESS_PRIVATE() providing the same guarantees as
> WRITE_ONCE(), therefore I think this also needs to be changed. I'll
> need to introduce something like vm_flags_reset_once() and use it
> here. vm_flags_reset_once() would do WRITE_ONCE() and otherwise would
> be identical to vm_flags_reset().
>
> I'll post a new version with the fixes later today.
>
> Thanks for the review!
> Suren.

Thanks for quick reply!

Andrew's fix and the new patch looks good to me.
with these two things addressed:

Reviewed-by: Hyeonggon Yoo <42.hyeyoo@...il.com>


Regards,
Hyeonggon