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

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Aug 1, 2025 at 2:09 AM Vlastimil Babka <vbabka@suse.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@suse.cz>
>
> IIRC you considered it yourself, I just convinced you to try :)
>
> > Signed-off-by: Suren Baghdasaryan <surenb@google.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@suse.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;
> >  }