Re: [PATCH] mm: fix vma_start_write_killable() signal handling

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Nov 26, 2025 at 7:20 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Nov 26, 2025 at 03:05:44PM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 26, 2025 at 03:36:46PM +0100, Vlastimil Babka wrote:
> > > On 11/26/25 5:28 AM, Suren Baghdasaryan wrote:
> > > >> Suren, Liam, Vlastimil, Lorenzo ... none of you spotted this bug.
> > > >
> > > > Doh! This is embarassing...
> > >
> > > Hand-rolled synchronization primitives are wonderful, aren't they?
> >
> > That's why I liked the original approach of just using rwsems.  I
> > mst admit to having not paid attention to this recently so I don't
> > know what motivated the change.

That made it simpler to add SLAB_TYPESAFE_BY_RCU for vm_area_structs
which improved the performance of allocating these structs during
calls like mmap and fork.

> >
> > > > Wait, why do we consider this as a successful acquisition? The
> > > > vm_refcnt is 0, so this is similar situation to an earlier:
> > > >
> > > > if (!refcount_add_not_zero(VMA_LOCK_OFFSET, &vma->vm_refcnt))
> > > >         return 0;
> > >
> > > But this means "vma is not attached" not "we failed to lock it".
> > >
> > > > IOW, the vma is not referenced, so we failed to lock it. I think the
> > > > fix should be:
> > > >
> > > >         if (err) {
> > > > +               if (refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt)) {
> > > > +                       /* Oh cobblers.  While we got a fatal signal, we
> > > > +                        * raced with the last user.  VMA is not referenced,
> > > > +                        * fail to lock it.
> > > > +                        */
> > > > +                       err = 0;
> > >
> > > Returning 0 in this situation therefore wouldn't be correct.
> > >
> > > AFAIU since we started with attached vma above, it's not possible that
> > > the refcount_sub_and_test here will drop the refcnt to zero. We could
> > > just WARN_ON_ONCE() on the result (in a way to make also the
> > > __must_check happy) and then can return err below.
> >
> > But how do we know that we started with an attached VMA?  Maybe the VMA
> > was in the process of being detached and still has readers?
>
> So we're talking about:
>
> vma_mark_deteched()
> -> refcount_dec_and_test() [ ref count is zero ]
> -> __vma_enter_locked()
>
> (meanwhile...)
>
> -> reader attempts to read
>   -> optimistic check doesn't successfully find write locked VMA
>   -> __refcount_inc_not_zero_limited_acqure() somehow doesn't notice 0 refcount and increments
>     (??? how)
>
> (back to vma_mark_attached() -> __vma_enter_locked())
>
> -> refcount_add_not_zero() returns true
>
> [ process gets fatal signal ]
>
> -> rcuwait_wait_event() errors out
> -> oopsies need to do something, maybe [VM_]WARN_ON() not right?
>
> Correct me if the above is wrong.
>
> I mean is any of this actually possible...?
>
> Seems dubious. But I guess right now we assume it _is_ possible. What a mess!
>
> (Again I wonder why we made our lives so difficult here)
>
> Anyway even if we are midway through a detach, the detach is ostensibly waiting
> for the readers to go away, and our reader is about to go away anyway, but the
> process has a fatal signal so do we even care?
>
> I actually wonder if a WARN_ON() is warranted to see if this even ever
> happens...
>
> OK just going to reattach... my head which just exploded from the above :P

We are overthinking this. Vlastimil is right. If we entered with an
attached VMA (which is the case due to this check:
https://elixir.bootlin.com/linux/v6.18-rc7/source/mm/mmap_lock.c#L60)
then the only function that can detach the VMA from under us is
vma_mark_detached() but that function can't race with
__vma_enter_locked() because both functions are required to hold
mmap_write_lock. vma_mark_detached() has an explicit
vma_assert_write_locked(vma) check (which implies mmap_write_lock) and
vma_start_write() calls __is_vma_write_locked() which does
mmap_assert_write_locked(vma->vm_mm). We can add a comment here or an
mmap_assert_write_locked(vma->vm_mm); at the beginning of the
__vma_enter_locked() to make that obvious.

> Cheers, Lorenzo