linux-kernel - Re: [PATCH 2/2] mm: change vma_start_read() to drop RCU lock on failure

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] [day] [month] [year] [list]

Message-ID: <4dd77a95-1528-4289-b94d-a4da6b6975cf@lucifer.local>
Date: Fri, 1 Aug 2025 11:41:43 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: akpm@...ux-foundation.org, jannh@...gle.com, Liam.Howlett@...cle.com,
        vbabka@...e.cz, pfalcato@...e.de, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mm: change vma_start_read() to drop RCU lock on
 failure

On Thu, Jul 31, 2025 at 07:06:41AM -0700, Suren Baghdasaryan wrote:
> On Thu, Jul 31, 2025 at 4:49 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > So this patch is broken :P
>
> Ugh, sorry about that. I must have had lockdep disabled when testing this...

No worries, curiously it doesn't seem to be lockdep that picked this up. RCU
tiny doesn't seem to do this so perhaps you somehow had this enabled? Not sure,
strange one!

[snip]

> > > Suggested-by: Vlastimil Babka <vbabka@...e.cz>
> > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > ---
> > >  mm/mmap_lock.c | 76 +++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 44 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > > index 10826f347a9f..0129db8f652f 100644
> > > --- a/mm/mmap_lock.c
> > > +++ b/mm/mmap_lock.c
> > > @@ -136,15 +136,21 @@ void vma_mark_detached(struct vm_area_struct *vma)
> > >   * Returns the vma on success, NULL on failure to lock and EAGAIN if vma got
> > >   * detached.
> > >   *
> > > - * WARNING! The vma passed to this function cannot be used if the function
> > > - * fails to lock it because in certain cases RCU lock is dropped and then
> > > - * reacquired. Once RCU lock is dropped the vma can be concurently freed.
> > > + * WARNING! On entrance to this function RCU read lock should be held and it
> > > + * is released if function fails to lock the vma, therefore vma passed to this
> > > + * function cannot be used if the function fails to lock it.
> > > + * When vma is successfully locked, RCU read lock is kept intact and RCU read
> > > + * session is not interrupted. This is important when locking is done while
> > > + * walking the vma tree under RCU using vma_iterator because if the RCU lock
> > > + * is dropped, the iterator becomes invalid.
> > >   */
> >
> > I feel like this is a bit of a wall of noise, can we add a clearly separated line like:
> >
> >         ...
> >         *
> >
> >         * IMPORTANT: RCU lock must be held upon entering the function, but
> >         *            upon error IT IS RELEASED. The caller must handle this
> >         *            correctly.
> >         */
>
> Are you suggesting to replace my comment or amend it with this one? I
> think the answer is "replace" but I want to confirm.

Oh no, amend is fine!

I see you sent a v2 so will check that :P

[snip]

> > >  /*
> > > @@ -223,8 +233,8 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > >       MA_STATE(mas, &mm->mm_mt, address, address);
> > >       struct vm_area_struct *vma;
> > >
> > > -     rcu_read_lock();
> > >  retry:
> > > +     rcu_read_lock();
> > >       vma = mas_walk(&mas);
> > >       if (!vma)
> > >               goto inval;
> >                 ^
> >                 |---- this is incorrect, you took the RCU read lock above, but you don't unlock... :)
> >
> > You can fix easily with:
> >
> >         if (!vma) {
> >                 rcu_read_unlock();
> >                 goto inval;
> >         }
> >
> > Which fixes the issue locally for me.
>
> Yes, I overlooked that here we did not yet attempt to lock the vma. Will fix.
> Thanks!

Yeah easy one to miss!

This stuff is very subtle, but I hope that when I finally get VMA lock 'extra
docs' done (really written for us, or rather mm kernel devs, than for kernel
devs more broadly) we can help address some of the subtle aspects.

In reality of course, my main motivation with that is to ensure _I_ have docs I
can go reference when needed :P

[snip]

Cheers, Lorenzo