Re: [PATCH v3 RESEND] x86/mm: Fix PAT bit missing from page protection modify mask

[Edgecombe, Rick P]

On Mon, 2023-07-10 at 09:36 +0200, Janusz Krzysztofik wrote:
> The issue needs to be fixed by including _PAGE_PAT bit into a bitmask
> used
> by pgprot_modify() for selecting bits to be preserved.  We can do
> that
> either internally to pgprot_modify() (as initially proposed), or by
> making
> _PAGE_PAT a part of _PAGE_CHG_MASK.  If we go for the latter then,
> since
> _PAGE_PAT is the same as _PAGE_PSE, we need to note that
> _HPAGE_CHG_MASK
> -- a huge pmds' counterpart of _PAGE_CHG_MASK, introduced by commit
> c489f1257b8c ("thp: add pmd_modify"), defined as (_PAGE_CHG_MASK |
> _PAGE_PSE) -- will no longer differ from _PAGE_CHG_MASK.  If such
> modification of _PAGE_CHG_MASK was irrelevant to its users then one
> might
> wonder why that new _HPAGE_CHG_MASK symbol was introduced instead of
> reusing the existing one with that otherwise irrelevant bit
> (_PAGE_PSE in
> that case) added.
> 
> Assume that adding _PAGE_PAT to _PAGE_CHG_MASK doesn't break
> pte_modify()
> and its users, and go for it.  Also, add _PAGE_PAT_LARGE to
> _HPAGE_CHG_MASK for symmetry.  For better clarity, split out common
> bits
> from both symbols to another one and use it together with specific
> bits
> when defining the masks.

I think this whole entanglement is a bit confusing, but not
functionally broken.

The problem is kind of that the existing code assumes that all vma-
>vm_page_prot relevant prot bit positions are identical between PTE and
PMDs. The bug is that _PAGE_PAT is not treated as relevant, but it
actually is. So fixing it breaks the assumption.

Not trying to suggest we shouldn't do the simple fix here, but I wonder
if it is worth clearing it up further?

I think at least we should include a comment around _PAGE_CHG_MASK or
_PAGE_PAT as to why it is assumed that this will not cause problems.
This way, someone in the future can see the error in the reasoning if
something changes. Would something like this be useful and correct
reasoning?

/*
 * Bit 7 has different meanings for PTEs and PMDs. On PTEs it is the
 * PAT bit, and on PMDs it is the PSE bit. This creates some confusing 
 * entanglement in code that operates page table bits that end up in
 * both PTEs and PMDs.
 *
 * vma->vm_page_prot is used for constructing both PTE's and huge
 * page PMDs. This member is set via vm_pgprot_modify() which, despite
 * vma->vm_page_prot being used to set huge pages as well, only filters
 * bits with _PAGE_CHG_MASK (actually in pgprot_modify()). This means
 * by having _PAGE_PAT in _PAGE_CHG_MASK, _PAGE_PSE will also be 
 * preserved.
 *
 * This should be harmless because vma->vm_page_prot is only applied to
 * leaf page table entries. In the case of _PAGE_PAT in
 * vma->vm_page_prot being inadvertently applied to a huge page, this
 * is fine because _PAGE_PSE will already be being set on a huge page.
 * It is expected that code should not try to add _PAGE_PSE to
 * vma->vm_page_prot, so there shouldn't be the case of inadvertently
 * setting _PAGE_PAT in a PTE. So the saving grace is that PSE is
 * *always* set on PMDs that add bits derived from vma->vm_page_prot.
 */