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

[Vlastimil Babka <vbabka@suse.cz>]

On 11/26/25 7:43 PM, Suren Baghdasaryan wrote:
> On Wed, Nov 26, 2025 at 6:28 PM Matthew Wilcox <willy@infradead.org> wrote:
>>
>> On Wed, Nov 26, 2025 at 06:06:53PM +0000, Lorenzo Stoakes wrote:
>>> On Wed, Nov 26, 2025 at 05:44:58PM +0000, Matthew Wilcox (Oracle) wrote:
>>>> If we get a signal, we need to restore the vm_refcnt.  We don't think
>>>> that the refcount can actually be decremented to zero here as it
>>>> requires the VMA to be detached, and the vma_mark_detached() uses
>>>> TASK_UNINTERRUPTIBLE.  However, that's a bit subtle, so handle it
>>>> as if the refcount was zero at the start of this function.
>>>>
>>>> Reported-by: syzbot+5b19bad23ac7f44bf8b8@syzkaller.appspotmail.com

Closes: https://lore.kernel.org/all/69252076.a70a0220.d98e3.009b.GAE@google.com/

Unfortunately the reported-by code doesn't work as message id

>>>> 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>
> 
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> 
>>>> ---
>>>>  mm/mmap_lock.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
>>>> index e6e5570d1ec7..3c9bf2f96280 100644
>>>> --- a/mm/mmap_lock.c
>>>> +++ b/mm/mmap_lock.c
>>>> @@ -74,6 +74,14 @@ 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)) {
>>>
>>> Really think we should WARN_ON_ONCE() as Vlasta suggested.

Thanks Lorenzo!

Can we at least add a "WARN_ON_ONCE(!detaching);" before "err = 0;".

>>> It's an 'impossible' situation so we should make that clear. And we should
>>> find out about it if the impossible happens... :)
>>
>> It's only "impossible" currently due to some fairly esoteric reasoning.
>> As far as _this_ function is concerned, it's entirely possible.
>> I don't want to leave this trap for the next person who calls
>> __vma_enter_locked(TASK_KILLABLE).
>>
>>>> +                   /*
>>>> +                    * We got a fatal signal, but the last reader went
>>>> +                    * away as well.  Resolve the race in favour of
>>>
>>> This is very subtle, I don't think this really explains this clearly
>>> enough.
>>>
>>> Maybe put something like:
>>>
>>>       /* Couldn't wait on readers probably due to a fatal signal, so unlock. */
>>>
>>> Before the refcount_sub_and_test()
>>
>> I think this falls into the "saying what you're doing, not why
>> you're doing it" trap.  Whereas my comment is at a higher level --
>> there's a race where both exit conditions are true at the same time.
>> The rcuwait_wait_event() picked one option, but we would rather resolve
>> the race in the opposite direction.
>>
>>> And:
>>>
>>>       /* Shouldn't be possible - VMA entirely detached, so treat it as such. */
>>>
>>> Before err = 0?
>>
>> Again though, saying it's "not possible" relies on knowing all the
>> callers of this function behave a particular way, and there's no
>> guarantee they'll continue to do so.
> 
> I agree with Matthew. Returning 0 here would work correctly even if
> vma_mark_detached() starts using TASK_INTERRUPTIBLE and 0 becomes
> possible. The comment also seems appropriate to me.
> Thanks!
> 
>>