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

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Nov 26, 2025 at 8:01 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Nov 26, 2025 at 07:49:13AM -0800, Suren Baghdasaryan wrote:
> > 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.
>
> Hmm, we doubled down :)
>
> Hope the wins were worth the complexity, but obviously the horse has bolted
> and I should have asked more about it at the time...
>
> >
> > > >
> > > > > > 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
>
> Watch Vlastimil frame this... ;)
>
> > 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
>
> We hold the mmap/VMA write lock yes, but as per the comment in
> vma_mark_detached() + above list readers can spuriously increment the
> refcount (ostensibly) as per:
>
>                 /* Wait until vma is detached with no readers. */
>
> So doesn't that mean we can hit the issue Matthew raised?
>
> Otherwise there would be no need to do this dance in vma_mark_detached()
> anyway?
>
> Maybe I'm missing something.

No, you are right. I realized after sending the reply this temporary
refcount from the reader can be dropped from under the writer. Was in
the process of analyzing these paths when Vlastimil's reply came in.
I'll spend some more time stairing at it to see if we are missing
anything else.

>
> > __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
> >
>
> Thanks, Lorenzo