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

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

Feel free to send what you have below and you can add:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

On Wed, Nov 26, 2025 at 10:09:36PM +0000, Matthew Wilcox wrote:
> 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.

Ack insert moany waffle about this impl. being confusing for _everybody_ here :)

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

Right, sure.

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

Yup, but not the unlocking... I can address that later in the series I'm
inevitably going to end up sending to improve this file :)

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

OK this is fine, respin or ask Andrew to fix-patch and have a tag :)

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

Fine, sure.

Cheers, Lorenzo