Re: [PATCH v10 12/18] mm: replace vm_lock and detached flag with a reference count

[Suren Baghdasaryan]

On Thu, Feb 20, 2025 at 10:53 AM Heiko Carstens <hca@xxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 13, 2025 at 02:46:49PM -0800, Suren Baghdasaryan wrote:
> ...
> > While this vm_lock replacement does not yet result in a smaller
> > vm_area_struct (it stays at 256 bytes due to cacheline alignment), it
> > allows for further size optimization by structure member regrouping
> > to bring the size of vm_area_struct below 192 bytes.
> >
> > Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > ---
> > Changes since v9 [1]:
> > - Use __refcount_inc_not_zero_limited_acquire() in vma_start_read(),
> > per Hillf Danton
> > - Refactor vma_assert_locked() to avoid vm_refcnt read when CONFIG_DEBUG_VM=n,
> > per Mateusz Guzik
> > - Update changelog, per Wei Yang
> > - Change vma_start_read() to return EAGAIN if vma got isolated and changed
> > lock_vma_under_rcu() back to detect this condition, per Wei Yang
> > - Change VM_BUG_ON_VMA() to WARN_ON_ONCE() when checking vma detached state,
> > per Lorenzo Stoakes
> > - Remove Vlastimil's Reviewed-by since code is changed
>
> This causes crashes (NULL pointer deref) with linux-next when running
> the ltp test suite; mtest06 (mmap1) test case.
>
> The bug seems to be quite obvious:
>
> > @@ -6424,15 +6492,18 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >       if (!vma)
> >               goto inval;
> >
> > -     if (!vma_start_read(vma))
> > -             goto inval;
> > +     vma = vma_start_read(vma);
> > +     if (IS_ERR_OR_NULL(vma)) {
>             ^^^^^^^^^^^^^^^^^^^
> > +             /* Check if the VMA got isolated after we found it */
> > +             if (PTR_ERR(vma) == -EAGAIN) {
> > +                     vma_end_read(vma);
>                         ^^^^^^^^^^^^^^^^

Doh! Thanks for reporting! I'll post a fix shortly.