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

[Vlastimil Babka <vbabka@suse.cz>]

On 11/26/25 3:26 PM, Suren Baghdasaryan wrote:
> On Tue, Nov 25, 2025 at 8:28 PM Suren Baghdasaryan <surenb@google.com> wrote:
>>
>> On Tue, Nov 25, 2025 at 7:44 PM Matthew Wilcox (Oracle)
>> <willy@infradead.org> wrote:
>>>
>>> If we get a signal, we need to restore the vm_refcnt.  The wrinkle in
>>> that is that we might be the last reference.  If that happens, fix the
>>> refcount to look like we weren't interrupted by a fatal signal.
>>>
>>> Reported-by: syzbot+5b19bad23ac7f44bf8b8@syzkaller.appspotmail.com
>>> Fixes: 2197bb60f890 ("mm: add vma_start_write_killable()")
>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>> Cc: Suren Baghdasaryan <surenb@google.com>
>>> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>> Andrew, since the vma_start_write_killable() patch is in mm-stable,
>>> I don't think you can put this in as a fixup, right?
>>>
>>> Suren, Liam, Vlastimil, Lorenzo ... none of you spotted this bug.
>>
>> Doh! This is embarassing...
>>
>>> Any other stupid thing I've done?  And am I doing the right thing
>>> with refcount_set()?
>>>
>>>  mm/mmap_lock.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
>>> index e6e5570d1ec7..71af7f0a5fe1 100644
>>> --- a/mm/mmap_lock.c
>>> +++ b/mm/mmap_lock.c
>>> @@ -74,9 +74,18 @@ static inline int __vma_enter_locked(struct vm_area_struct *vma,
>>>                    refcount_read(&vma->vm_refcnt) == tgt_refcnt,
>>>                    state);
>>>         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:
> 
> 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;

Not necessary IMHO. It could become pending right after we return 0, no
point in testing it here. We should only care about the signals in the
part where we sleep, i.e. rcuwait_wait_event().

>                  return 0;
> +        }
> 
>          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.
> +                         */
> +                }

Like I said in the other reply, the "no more users" should not happen,
so just WARN_ON_ONCE() on the result, but otherwise agreed.

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