On Tue, 25 Feb 2025 at 12:10, tip-bot2 for Matthew Wilcox (Oracle) <tip-bot2@xxxxxxxxxxxxx> wrote: > > We should, therefore, clear the _PAGE_DIRTY bit whenever we clear > _PAGE_RW. I opted to do it in the callers in case we want to use > __change_page_attr() to create shadow stacks inside the kernel at some > point in the future. Arguably, we might also want to clear _PAGE_ACCESSED > here. This explanation makes ZERO sense, and screams "this is a major bug" to me. If a page is dirty, it doesn't magically turn clean just because it becomes read-only. The dirty data remains and may need to be written back to memory. Imagine writing to a shared memory area, and then marking it all read-only after you're done. It's still dirty, even if it's read-only. Now, I don't actually expect this patch to be wrong, I'm literally just complaining about the explanation. Because the explanation is very lacking. That's particularly true for the __set_pages_np() case which also clears _PAGE_PRESENT, because then the whole shadow stacks explanation flies right out the window: the shadow stack rules simply do NOT APPLY to non-present pte's in the first place. So honestly, I think this wants an explanation for why it's actually a safe change, and how the dirty bit has been saved before the operation. Linus
![]() |