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

[Janusz Krzysztofik]

Hi Rick,

On Monday, 10 July 2023 19:58:07 CEST Edgecombe, Rick P wrote:
> 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.

AFAICU, your concern is different than mine was.  I was wondering if by adding 
_PAGE_PAT to _PAGE_CHG_MASK we didn't break PTEs in pte_modify(), while you 
seem to be rather thinking of potential breakage of PMDs in pgprot_modify().

>  *
>  * 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.
>  */
> 

I could add your comment and resubmit, but my experience from communication 
with audience of this patch tells me that silence means rather no acceptance.

Thanks,
Janusz