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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Nov 26, 2025 at 06:26:26AM -0800, Suren Baghdasaryan wrote:
> > >         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.  Pretend we didn't notice
> > > +                        * the signal
> > > +                        */
> > > +                       refcount_set(&vma->vm_refcnt, VMA_LOCK_OFFSET);
> > > +                       goto acquired;
> >
> > 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;
> >
> > IOW, the vma is not referenced, so we failed to lock it. I think the
> > fix should be:

Yes, I also wondered about doing it that way.  Of course, we hold the
write lock, and we found the VMA, so I don't think this can actually
occur?

> After sleeping on it, I don't think we should be masking EINTR error.
> __vma_enter_locked() result might be the only place where an outer
> loop is checking for fatal signals, so returning "failure to lock"
> instead of -EINTR might cause the loop to continue. I think this fix
> would be better:
> 
>           * If vma is detached then only vma_mark_attached() can raise the
>           * vm_refcnt. mmap_write_lock prevents racing with vma_mark_attached().
>           */
> -        if (!refcount_add_not_zero(VMA_LOCK_OFFSET, &vma->vm_refcnt))
> +        if (!refcount_add_not_zero(VMA_LOCK_OFFSET, &vma->vm_refcnt)) {
> +                if (fatal_signal_pending(current))
> +                        return -EINTR;

This part seems too much to me.  We don't need to check for signals
often, just when we'd otherwise sleep.

>          rwsem_acquire(&vma->vmlock_dep_map, 0, 0, _RET_IP_);
>          err = rcuwait_wait_event(&vma->vm_mm->vma_writer_wait,
>                     refcount_read(&vma->vm_refcnt) == tgt_refcnt,
>                     state);
>          if (err) {
> +                if (refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt)) {
> +                        /*
> +                         * No more users but fatal signal is present,
> +                         * still return the error.
> +                         */
> +                }

Umm.  Does the last owner of a vm_refcnt not need to do anything like
free the vma?

>                  rwsem_release(&vma->vmlock_dep_map, _RET_IP_);
>                  return err;
>          }