Re: [PATCH 11/12] mm/madvise: send UFFD_EVENT_REMOVE when installing guard regions

[Tal Zussman]

On 11/12/26 6:03 AM, Lorenzo Stoakes (ARM) wrote:
> On Wed, Nov 11, 2026 at 12:55:19PM -0500, Tal Zussman wrote:
>> MADV_GUARD_INSTALL zaps whatever is mapped in the range before
>> installing the guard markers. However, unlike MADV_DONTNEED and
>> MADV_REMOVE, madvise_guard_install() does not call userfaultfd_remove(),
>> so the userfaultfd manager is not told that the pages are gone.
>>
>> On an anonymous VMA, the zap drops uffd-wp and RWP protection along with
>> the pages. Writes and accesses to the range after the guard is removed
>> are then not reported, and the manager cannot know that it has to
>> protect the range again.
>>
>> To fix it, send UFFD_EVENT_REMOVE before zapping. Guard install on a VMA
>> registered with userfaultfd already takes the mmap lock instead of the
>> VMA lock, so we can drop the lock for the event and look the VMA up
>> again afterwards.
>>
>> As with MADV_DONTNEED, the call can now return -ENOMEM if the VMA goes
>> away while the lock is dropped, or -EINVAL if it is replaced by a VMA
>> that does not support guard regions.
>>
>> Fixes: 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism")
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/linux- 
> mm/3f4b0d1b.bbf84.1a1c55cb666.Coremail.18670333770@xxxxxxx/__;!!BDUfV1Et5lrpZQ! 
> T5BLvK-D7GgO7Y87n7ULpXAEzR6V57uNMU7saDR9ShW05mDBxMyqFA4DD22Nbycn_r7rhjPtEZp8gBAN$ <https://urldefense.com/v3/__https://lore.kernel.org/linux-mm/3f4b0d1b.bbf84.1a1c55cb666.Coremail.18670333770@xxxxxxx/__;!!BDUfV1Et5lrpZQ!T5BLvK-D7GgO7Y87n7ULpXAEzR6V57uNMU7saDR9ShW05mDBxMyqFA4DD22Nbycn_r7rhjPtEZp8gBAN$>
> 
> This report was super-synthetic AI slop
> 

Will drop it

>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> 
> Sorry I hate this.
> 

Don't worry, me too...

> Since this was already silently broken, I think the right solution is to simply
> disallow guard region installation for uffd-armed VMAs.
> 
> I had the AI go look for users, and it seems nobody is relying on this (glibc,
> android, CRIU, etc.), and there's really no sensible world in which somebody's
> combining the two. So I think fine to do that.
> 

What about CRIU restoring an application that uses guard regions?
CRIU registers the task's memory with uffd MISSING mode for lazy restore, and
if the application (e.g., glibc) is using guard regions, this could break.
I believe glibc falls back to mprotect() after guard installation failure,
but I'm not sure about other users. I agree that there's no sensible reason for
a single application to combine these two, but uffd manager mode is where we
get these fun combinations...

Re: "silently broken", WP and RWP are more broken here than MISSING and
MINOR modes (due to the issues in the other two guard patches). MINOR and
MISSING mode already work today, modulo this event notification. If the CRIU
issue above matters, disallowing guard regions on WP/RWP VMAs could be a good
middle ground (and of course this should all be documented somewhere...).
Otherwise disallowing guard regions on uffd entirely sounds reasonable to me.

> It was simply an oversight (reflex action is to disable new features for hugetlb
> + uffd)
> 
> I'll send a patch separate from this, please drop it from the series.
> 

Will do!

>> ---
>>  mm/madvise.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index e8b05dc899f1..6413f0e65381 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1166,6 +1166,29 @@ static long madvise_guard_install(struct madvise_behavior *madv_behavior)
>>  	if (!is_valid_guard_vma(vma, /* allow_locked = */false))
>>  		return -EINVAL;
>>
>> +	/*
>> +	 * Installing guards zaps the range, so report it to a userfaultfd
>> +	 * manager like MADV_DONTNEED does. The VMA can change while the mmap
>> +	 * lock is dropped for the event, so look it up again.
>> +	 */
>> +	if (!userfaultfd_remove(vma, range->start, range->end)) {
>> +		mark_mmap_lock_dropped(madv_behavior);
>> +		mmap_read_lock(madv_behavior->mm);
>> +		madv_behavior->vma = vma = vma_lookup(madv_behavior->mm,
>> +						      range->start);
>> +		if (!vma)
>> +			return -ENOMEM;
>> +		if (!is_valid_guard_vma(vma, /* allow_locked = */false))
>> +			return -EINVAL;
>> +		/*
>> +		 * The VMA may have been split while the lock was dropped.
>> +		 * Handle the part in this VMA. The remainder is handled by the
>> +		 * next iteration of madvise_walk_vmas().
>> +		 */
>> +		if (range->end > vma->vm_end)
>> +			range->end = vma->vm_end;
>> +	}
>> +
> 
> (separate rant - that we have to a. know to do it and b. write duplicative
> open-coded crap like this is a perfect example of why uffd is such an utterly
> broken mm feature that should never have been merged...)
> 

But where would be the fun in that... ;)

>>  	/*
>>  	 * Set atomically under read lock. All pertinent readers will need to
>>  	 * acquire an mmap/VMA write lock to read it. All remaining readers may
>>
>> --
>> 2.39.5
>>
> 
> --
> Cheers, Lorenzo
>