Re: [PATCH v2 01/16] mm: correctly handle UFFD PTE markers

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

On Mon, Nov 10, 2025 at 12:26:26AM +0800, Lance Yang wrote:
> > @@ -175,8 +186,8 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
> >   		pte_t pte = ptep_get(ptep);
> >   		step = 1;
> > -		/* We need to do cache lookup too for pte markers */
> > -		if (pte_none_mostly(pte))
> > +		/* We need to do cache lookup too for UFFD pte markers */
> > +		if (pte_none(pte) || is_uffd_pte_marker(pte))
>
> Seems like something is changed, new is_uffd_pte_marker check will
> miss non-UFFD markers (like guard markers) , and then would fall
> through to the swap entry logic to be misreported as resident by
> mincore_swap().

I intentionally changed cases that seemed to be explicitly wanting to only check
for is_uffd_pte_marker().

The issue with markers is it was first implemented on the assumption that it was
only one kind (UFFD WP) then other markers were added without correction.

Since we explicitly test for the softleaf is swap case I assumed we were good
(we check for softleaf entries explicitly):

	/*
	 * Shmem mapping may contain swapin error entries, which are
	 * absent. Page table may contain migration or hwpoison
	 * entries which are always uptodate.
	 */
	if (!leafent_is_swap(entry))
		return !shmem;

But obviously didn't read that carefully enough - mincore assumes literally all
soft leaf entries can be considered present for not-shmem and shmem would only
have no-longer-exists swapin error entries...

Really that function needs refactoring and the is swap check put higher.

But TL;DR you're right I"ll send a fixpatch...

>
> ```
> 		/* We need to do cache lookup too for UFFD pte markers */
> 		if (pte_none(pte) || is_uffd_pte_marker(pte))
> 			__mincore_unmapped_range(addr, addr + PAGE_SIZE,
> 						 vma, vec);
> 		else if (pte_present(pte)) {
> 			unsigned int batch = pte_batch_hint(ptep, pte);
>
> 			if (batch > 1) {
> 				unsigned int max_nr = (end - addr) >> PAGE_SHIFT;
>
> 				step = min_t(unsigned int, batch, max_nr);
> 			}
>
> 			for (i = 0; i < step; i++)
> 				vec[i] = 1;
> 		} else { /* pte is a swap entry */
> 			*vec = mincore_swap(pte_to_swp_entry(pte), false);
> 		}
> ```
>
> Wouldn't the generic is_pte_marker() be safer here?

pte_is_marker() now :) I fixed the silly naming inconsistency...

Cheers, Lorenzo