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
|