Re: [RFC 2/2] x86/mm/cpa: drop pgprot_clear_protnone_bits()

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Tue, Jun 14, 2022 at 06:23:43PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-06-14 at 15:53 +0900, Hyeonggon Yoo wrote:
> > On Tue, Jun 14, 2022 at 03:39:33PM +0900, Hyeonggon Yoo wrote:
> > > commit a8aed3e0752b4 ("x86/mm/pageattr: Prevent PSE and GLOABL
> > > leftovers
> > > to confuse pmd/pte_present and pmd_huge") made CPA clear
> > > _PAGE_GLOBAL when
> > > _PAGE_PRESENT is not set. This prevents kernel crashing when kernel
> > > reads
> > > a page with !_PAGE_PRESENT and _PAGE_PROTNONE (_PAGE_GLOBAL). And
> > > then it
> > > set _PAGE_GLOBAL back when setting _PAGE_PRESENT again.
> > > 
> > > After commit d1440b23c922d ("x86/mm: Factor out pageattr
> > > _PAGE_GLOBAL
> > > setting") made kernel not set unconditionally _PAGE_GLOBAL, pages
> > > lose
> > > global flag after _set_pages_np() and _set_pages_p() are called.
> > > 
> > > But after commit 3166851142411 ("x86: skip check for spurious
> > > faults for
> > > non-present faults"), spurious_kernel_fault() does not confuse
> > > pte/pmd entries with _PAGE_PROTNONE as present anymore. So simply
> > > drop pgprot_clear_protnone_bits().
> > 
> >  
> > Looks like I forgot to Cc: Andrea Arcangeli <aarcange@redhat.com>
> > 
> > Plus I did check that kernel does not crash when reading from/writing
> > to
> > non-present pages with this patch applied.
> 
> Thanks for the history.
> 
> I think we should still fix pte_present() to not check prot_none if the
> user bit is clear.

I tried, but realized it wouldn't work :(

For example, when a pte entry is used as swap entry, _PAGE_PRESENT is
cleared and _PAGE_PROTNONE is set.

And other bits are used as type and offset of swap entry.
In that case, _PAGE_BIT_USER bit does not represent _PAGE_USER.
It is just one of bits that represents type of swap entry.

So checking if _PAGE_PROTNONE set only when _PAGE_USER is set
will confuse some swap entries as non-present.

> The spurious fault handler infinite loop may no
> longer be a problem, but pte_present() still would return true for
> kernel NP pages, so be fragile. Today I see at least the oops message
> and memory hotunplug (see remove_pagetable()) that would get confused.

As explained above, I don't think it's possible to make pte_present() 
accurate for both kernel and user ptes.

Maybe we can implement pte_present_kernel()/pte_present_user()
for when kernel knows it is user or kernel pte.

or pte_present_with_address(pte, address) if we don't
know it is user pte or kernel pte.

-- 
Thanks,
Hyeonggon