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
