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

[Matthew Wilcox <willy@infradead.org>]

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

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.