On Wed, 2023-01-04 at 14:25 +0100, Borislav Petkov wrote: > On Tue, Dec 27, 2022 at 11:31:37PM +0000, Edgecombe, Rick P wrote: > > The comment is referring to the dirty bits possibly coming from > > newprot, > > Ah right, ofc. > > > but looking at it now I think the code was broken trying to > > fix the recent soft dirty test breakage. Now it might lose pre- > > existing > > dirty bits in the pte unessarily... I think. > > Right, does this code need to be simplified? > > I.e., match the shadow stack PTE (Write=0,Dirty=1) and handle that in > a separate > helper? > > So that the flows are separate. I'm not a mm guy but this function > makes my head > hurt - dunno about other folks. :) Yea, the whole Write=0,Dirty=1 thing has been a bit of a challenge to make clear in the MM code. Dave had suggested a sketch here for pte_modify(): https://lore.kernel.org/lkml/95299e90-245b-61c5-8ef0-5e6da3c37c5e@xxxxxxxxx/ The problem was that pte_mkdirty() also sets the soft dirty bit. So it did more than preserve the dirty bit - it also added on the soft dirty bit. I extracted a helper __pte_mkdirty() that can optionally not set the soft dirty bit. So then it looks pretty close to how Dave suggested: 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; }
