Re: [PATCH] x86/mm: Fix pmd_modify() dropping the dirty bit

[Edgecombe, Rick P]

On Thu, 2026-09-03 at 07:18 -0700, Dave Hansen wrote:
> On 9/3/26 04:54, Kiryl Shutsemau wrote:
> > > Closes: https://lore.kernel.org/r/CAJxLxMUGu1-L+O_nAONOwOXnS=cNbNApCWqdthRjd76LThtSPg@xxxxxxxxxxxxxx/
> > > Fixes: bb3aadf7d446 ("x86/mm: Start actually marking _PAGE_SAVED_DIRTY")
> > Hm. I don't understand why would this commit explicitly exclude
> > _PAGE_DIRTY from the mask:
> > 
> > -       val &= _HPAGE_CHG_MASK;
> > +       val &= (_HPAGE_CHG_MASK & ~_PAGE_DIRTY);
> > 
> > Rick, could you comment? It doesn't look like a typo.
> 
> My guess is that it's some remnant from an earlier version of the patch.
> The asymmetry with pte_modify() vs. pmd_modify() just can't be explained
> any other way. It _might_ have been some attempt to say, "Hey
> _PAGE_DIRTY is now a part of the pgprot_t since it's part of the
> 'permissions' of a shadow stack PTE" that got abandoned.
> 
> But I don't see anything wrong with the fix at all.
> 
> It does give me pause that this has been losing user data for so long,
> but it must just be a weird combination of features that few folks use
> together (huge pages + MADV_FREE).

Oof. Looking back through the patch history, the dirty bit used to be handled
separately, such that the stripping was needed. Like this:

 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 {
	pteval_t _page_chg_mask_no_dirty = _PAGE_CHG_MASK & ~_PAGE_DIRTY;
 	pteval_t val = pte_val(pte), oldval = val;
	pte_t pte_result;
 
 	/*
 	 * Chop off the NX bit (if present), and add the NX portion of
 	 * the newprot (if present):
 	 */
	val &= _page_chg_mask_no_dirty;
	val |= check_pgprot(newprot) & ~_page_chg_mask_no_dirty;
 	val = flip_protnone_guard(oldval, val, PTE_PFN_MASK);

	pte_result = __pte(val);

	/*
	 * Dirty bit is not preserved above so it can be done
	 * in a special way for the shadow stack case, where it
	 * may need to set _PAGE_COW. __pte_mkdirty() will do this in
	 * the case of shadow stack.
	 */
	if (pte_dirty(pte))
		pte_result = __pte_mkdirty(pte_result, false);

	return pte_result;
 }
 
 static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
 {
	pteval_t _hpage_chg_mask_no_dirty = _HPAGE_CHG_MASK & ~_PAGE_DIRTY;
 	pmdval_t val = pmd_val(pmd), oldval = val;
	pmd_t pmd_result;

	val &= _hpage_chg_mask_no_dirty;
	val |= check_pgprot(newprot) & ~_hpage_chg_mask_no_dirty;
 	val = flip_protnone_guard(oldval, val, PHYSICAL_PMD_PAGE_MASK);


	pmd_result = __pmd(val);

	/*
	 * Dirty bit is not preserved above so it can be done
	 * in a special way for the shadow stack case, where it
	 * may need to set _PAGE_COW. __pmd_mkdirty() will do this in
	 * the case of shadow stack.
	 */
	if (pmd_dirty(pmd))
		pmd_result = __pmd_mkdirty(pmd_result, false);

	return pmd_result;
 }

The dirty bit was removed from the pte, then handled separately to share logic
in the mkdirty helpers. During development it was changed to do the necessary
adjustments depending on the dirty bit in 'val', but the mask adjustment on the
pmd_modify() side didn't get updated. So I don't remember or see any intention
for the difference.

I can't find anything back then that would have prevented it. The bug cause and
fix looks correct to me.