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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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

It's an 'impossible' situation so we should make that clear. And we should
find out about it if the impossible happens... :)

> +			/*
> +			 * 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()

And:

	/* Shouldn't be possible - VMA entirely detached, so treat it as such. */

Before err = 0?

> +			 * the vma being detached.
> +			 */
> +			err = 0;

< pre-existing issue >

As discussed off-list, this name is horrible.

It's a value that returns 1 if attached and now locked, 0 if detached, or an
error code if the RCU waiter fails.

It's not an error code and this actually tripped me up reading this code.

We can address it elsewhere however.


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

Thanks, Lorenzo