Re: [RFC PATCH 2/2] powerpc/mm: Replace _PAGE_USER with _PAGE_PRIV

[Paul Mackerras <paulus@ozlabs.org>]

On Sat, Mar 05, 2016 at 01:59:02PM +0530, Aneesh Kumar K.V wrote:
> Paul Mackerras <paulus@ozlabs.org> writes:
> 
> > [ text/plain ]
> > On Fri, Feb 26, 2016 at 08:50:50AM +0530, Aneesh Kumar K.V wrote:
> >> _PAGE_PRIV means the page can be accessed only by kernel. This is done
> >> to keep pte bits similar to PowerISA 3.0 radix PTE format. User
> >> pages are now makred by clearing _PAGE_PRIV bit.
> >
> >
> 
> .....
> 
> >> @@ -40,6 +40,11 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >>  		if (unlikely(access & ~old_pte))
> >>  			return 1;
> >
> > This check is going to do a different thing now as far as
> > _PAGE_USER/_PAGE_PRIV is concerned: previously it would prevent a
> > non-privileged access to a privileged page from creating a HPTE, now
> > it prevents a privileged access to a non-privileged page from creating
> > a HPTE.  A privileged access means an access by the kernel to a high
> > address, and arguably we would never have a non-privileged PTE at a
> > high (i.e. kernel) address, but it's still a semantic change that
> > should have been flagged in the patch description.
> 
> 
> We don't set _PAGE_PRIVILGED when we have a privilged acess to a non
> privilged page. We set it as below (with updated comments)
> 
> 	/*
> 	 * We set _PAGE_PRIVILEGED only when
> 	 * kernel mode access kernel space.
> 	 *
> 	 * _PAGE_PRIVILGED is NOT set
> 	 * 1) when kernel mode access user space
> 	 * 2) user space access kernel space.
> 	 */
> 	if (!(msr & MSR_PR) && !(REGION_ID(ea) == USER_REGION_ID))
> 		access |= _PAGE_PRIVILEGED;

You're confusing a page in the user part of the address space with a
non-privileged page.  Now, we would certainly expect that all pages in
the user part of the address space would be non-privileged pages and
all pages in the kernel part would be privileged pages, but nothing
actually enforces that.  The semantic change is that if we did somehow
happen to have a non-privileged page (one without _PAGE_PRIVILEGED
set) in the kernel part of the address space, we can no longer access
it from the kernel.  Now you can argue that we never have
non-privileged pages in the kernel part of the address space, and so
the change doesn't matter.  That is probably a good argument, but you
do need to mention the change and make that argument in your patch
description.

Paul.