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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Nov 26, 2025 at 08:33:47PM +0000, Lorenzo Stoakes wrote:
> On Wed, Nov 26, 2025 at 07:44:17PM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 26, 2025 at 06:55:52PM +0000, Lorenzo Stoakes wrote:
> > > > 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).
> > >
> > > Calls __vma_enter_locked(TASK_KILLABLE) _when detaching_, otherwise
> > > refcount will always be >0.
> > >
> > > So we're only looking at us changing vma_mark_detached() to use
> > > TASK_KILLABLE.
> > >
> > > As this is such a subtle corner case I still think it warrants a
> > > warning. Or at least a VM_WARN_ON_ONCE(1).
> > >
> > > A killable detacher is, as Vlasta points out, kind of an unwise thing to do
> > > anyway right?
> >
> > I missed where that was said?
> 
> "Yeah I guess it's for the best to keep vma_mark_detached() use the
> TASK_UNINTERRUPTIBLE variant, maybe document why. Aborting the detaching
> would be counter productive."
> 
> https://lore.kernel.org/all/058f5858-f508-40f8-adfe-e5de78621d64@suse.cz/

I'm not entirely clear on why aborting a detach is always a bad idea,
but that's part of the MM I don't really understand.

> - A fatal signal arose (assuming nobody ever goes and changes
>   rcuwait_wait_event() to add more errors - very likely, not entirely certain
>   though, so perhaps 'an error that meant we couldn't wait'.)

It actually doesn't matter why we got an error.  We got an error.
But also the last reader went away.  So we're now in a state where we
would not have needed to sleep had we got here half a nanosecond later
than we did.

> Since you're concerned about the urgency, let me suggest a compromise:
> 
> 	/*
> 	 * We tried waiting on readers, but failed, likely due to a fatal
>          * signal arising. Unlock the VMA and check whether the VMA is
> 	 * detached.
> 	 */

I think the 'if (err)' is enough to tell the reader that we failed!

> 	if (refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt)) {
> 		/*
> 		 * If the VMA is now detached which means we lost a race.
> 		 * Let the caller know the VMA is detached.
> 		 */
> 		err = 0;
> 	}
> 
> That gives a _lot_ more information, keeps it relatively top-level, doesn't
> make undue assumptions etc.

Here's what I now have:

        if (err) {
                if (refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt)) {
                        /*
                         * The wait failed, but the last reader went away
                         * as well.  Tell the caller the VMA is detached.
                         */
                        WARN_ON_ONCE(!detaching);
                        err = 0;
                }

> > Are you satisfied with the WARN_ON(!detaching)?
> >
> 
> It'd be super weird to reach that code when not detaching so sure, think it
> should be VM_WARN_ON() though since the code would be horribly broken if
> that was not the case already no?

The other places in this file are WARN_ON_ONCE rather than VM_WARN*, so
keep it consistent.