Re: [PATCH] x86/mm/cpa: set PAGE_KERNEL in __set_pages_p()

[Tom Lendacky]

On 5/10/22 06:57, Hyeonggon Yoo wrote:
> On Mon, May 09, 2022 at 11:06:22AM -0500, Tom Lendacky wrote:
> > On 5/6/22 00:19, Hyeonggon Yoo wrote:
> > > __set_pages_np() not only clears _PAGE_PRESENT and _PAGE_RW, but also
> > > clears _PAGE_GLOBAL to avoid confusing _PAGE_GLOBAL as _PAGE_PROTNONE
> > > when the PTE is not present.
> > >
> > > Common usage for __set_pages_p() is to call it after __set_pages_np().
> > > Therefore calling __set_pages_p() after __set_pages_np() clears
> > > _PAGE_GLOBAL, making it unable to globally shared in TLB.
> > >
> > > As they are called by set_direct_map_{invalid,default}_noflush(),
> > > pages in direct map cannot be globally shared in TLB after being used by
> > > vmalloc, secretmem, and hibernation.
> > >
> > > So set PAGE_KERNEL isntead of __pgprot(_PAGE_PRESENT | _PAGE_RW) in
> > > __set_pages_p().
> > >
> > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> > > ---
> > >    arch/x86/mm/pat/set_memory.c | 2 +-
> > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > > index abf5ed76e4b7..fcb6147c4cd4 100644
> > > --- a/arch/x86/mm/pat/set_memory.c
> > > +++ b/arch/x86/mm/pat/set_memory.c
> > > @@ -2177,7 +2177,7 @@ static int __set_pages_p(struct page *page, int numpages)
> > >    	struct cpa_data cpa = { .vaddr = &tempaddr,
> > >    				.pgd = NULL,
> > >    				.numpages = numpages,
> > > -				.mask_set = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > > +				.mask_set = PAGE_KERNEL,
> >
> > With SME/SEV, this will also (unintentionally) set the encryption bit, so I
> > don't think this is correct.
>
> Thank you for catching this.
> It seems PAGE_KERNEL was too much for  __set_pages_p().
>
> I think __pgprot_mask(_PAGE_KERNEL | _PAGE_RW | _PAGE_GLOBAL) would be correct.
> Thoughts?

That should work, but you are counting on this function being called in a 
specific situation. Should this function ever be called from some other 
place in the kernel, in the future, that might inadvertently apply the 
global setting when it shouldn't.

I'm wondering if adding a specific helper that takes a boolean to indicate 
whether to set the global flag would be best. I'll let some of the MM 
maintainers comment about that.

Thanks,
Tom

> > Thanks,
> > Tom
> >
> > >    				.mask_clr = __pgprot(0),
> > >    				.flags = 0};