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

[Vlastimil Babka <vbabka@suse.cz>]

On 11/26/25 5:28 AM, Suren Baghdasaryan 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...

Hand-rolled synchronization primitives are wonderful, aren't they?

>> Any other stupid thing I've done?  And am I doing the right thing
>> with refcount_set()?

I think it's not wrong, but because it's dead code anyway. So it's
unnecessary.

>>  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;

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.

> +               }
>                 rwsem_release(&vma->vmlock_dep_map, _RET_IP_);
>                 return err;
>         }
> 
> 
>> +               }
>>                 rwsem_release(&vma->vmlock_dep_map, _RET_IP_);
>>                 return err;
>>         }
>> +acquired:
>>         lock_acquired(&vma->vmlock_dep_map, _RET_IP_);
>>
>>         return 1;
>> --
>> 2.47.2
>>