linux-kernel - Re: [PATCH v2 2/2] mm: change vma_start

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: <CAJuCfpHsdU5e7+f29ezDXus5-W7jT580YCY0LpErTfKJjeo8zQ@mail.gmail.com>
Date: Fri, 1 Aug 2025 08:30:45 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: akpm@...ux-foundation.org, jannh@...gle.com, Liam.Howlett@...cle.com, 
	lorenzo.stoakes@...cle.com, pfalcato@...e.de, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mm: change vma_start_read() to drop RCU lock on failure

On Fri, Aug 1, 2025 at 2:09 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 7/31/25 17:19, Suren Baghdasaryan wrote:
> > vma_start_read() can drop and reacquire RCU lock in certain failure
> > cases. It's not apparent that the RCU session started by the caller of
> > this function might be interrupted when vma_start_read() fails to lock
> > the vma. This might become a source of subtle bugs and to prevent that
> > we change the locking rules for vma_start_read() to drop RCU read lock
> > upon failure. This way it's more obvious that RCU-protected objects are
> > unsafe after vma locking fails.
> >
> > Suggested-by: Vlastimil Babka <vbabka@...e.cz>
>
> IIRC you considered it yourself, I just convinced you to try :)
>
> > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
>
> I thought we didn't need the drop rcu lock for -EAGAIN, but that would just
> made it more complex for little gain, so this looks good to me.

Yes, we technically don't but this way it's simpler to explain (drop
RCU on any failure).

>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
>
> Nit:
>
> > @@ -223,11 +227,13 @@ 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)
> > +     if (!vma) {
> > +             rcu_read_unlock();
> >               goto inval;
> > +     }
> >
> >       vma = vma_start_read(mm, vma);
> >       if (IS_ERR_OR_NULL(vma)) {
> > @@ -241,6 +247,9 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >               /* Failed to lock the VMA */
> >               goto inval;
> >       }
> > +
> > +     rcu_read_unlock();
>
> Would it make sense to put this under the comment below?
>
> > +
> >       /*
> >        * At this point, we have a stable reference to a VMA: The VMA is
> >        * locked and we know it hasn't already been isolated.
>
> Give it continues like this:
>
>          * From here on, we can access the VMA without worrying about which
>
>          * fields are accessible for RCU readers.

Yep, will change.
Thanks!

>
> > @@ -249,16 +258,14 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >        */
> >
> >       /* Check if the vma we locked is the right one. */
> > -     if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> > -             goto inval_end_read;
> > +     if (unlikely(address < vma->vm_start || address >= vma->vm_end)) {
> > +             vma_end_read(vma);
> > +             goto inval;
> > +     }
> >
> > -     rcu_read_unlock();
> >       return vma;
> >
> > -inval_end_read:
> > -     vma_end_read(vma);
> >  inval:
> > -     rcu_read_unlock();
> >       count_vm_vma_lock_event(VMA_LOCK_ABORT);
> >       return NULL;
> >  }