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

[Matthew Wilcox <willy@infradead.org>]

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?

> >
> > > > +			/*
> > > > +			 * 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.
> 
> I find your comment unclear, and I think it's too succinct. I was trying to
> provide the most succinct-yet-still-clear example, but if you prefer higher
> level you're going to need more detail here.
> 
> It assumes you 'just know' that:
> 
> - refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt) means unlock

Actually, I don't know that.  All I know is local to this function --
that's the value we added earlier before waiting; now we need to
subtract it since we're no longer waiting.

> - err can only be set due to a fatal signal in a non-uninterruptible task
>   mode

The comment says that in the first five words!

> - spurious readers can cause an incremented reference count

I don't know what a "spurious reader" is.  There was a reader when we
started waiting.  Now there isn't one.

> - that a race can exist between a spuriously raised reference count and the
>   previous reference count check between read above and refcount subtract here
> 
> - a reference count of 0 means detached
> 
> - err = 0 means we are treating this VMA as detached resolving this race
>   'in favour of' the VMA being detached.
> 
> Let's get some of this information in here please.

I don't think that here is the place to document these things!  And
certainly not in a patch that we're trying to get applied five days
before the merge window opens.  There's plenty of time to get the
comments and the variable names sorted out; can we focus on the right
way to fix this bug?

> Again I think we'd be better off with at least a VM_WARN_ON_ONCE() given
> this is a rather obscure corner case.

Are you satisfied with the WARN_ON(!detaching)?