[PATCH] powerpc: Fix invalid page flags in create TLB CAM pathfor PTE_64BIT

[Chen, Tiejun]

> -----Original Message-----
> From: 
> linuxppc-dev-bounces+tiejun.chen=windriver.com at lists.ozlabs.or
> g 
> [mailto:linuxppc-dev-bounces+tiejun.chen=windriver.com at lists.o
zlabs.org] On Behalf Of Scott Wood
> Sent: Friday, September 24, 2010 4:34 AM
> To: Gortmaker, Paul
> Cc: linuxppc-dev at lists.ozlabs.org
> Subject: Re: [PATCH] powerpc: Fix invalid page flags in 
> create TLB CAM pathfor PTE_64BIT
> 
> On Thu, 23 Sep 2010 16:10:15 -0400
> Paul Gortmaker <paul.gortmaker at windriver.com> wrote:
> 
> > So the possibility exists to wrongly assign the user 
> MAS3_U<RWX> bits 
> > to kernel (PAGE_KERNEL_X) address space via the following 
> code fragment:
> > 
> >         if (flags & _PAGE_USER) {
> >            TLBCAM[index].MAS3 |= MAS3_UX | MAS3_UR;
> >            TLBCAM[index].MAS3 |= ((flags & _PAGE_RW) ? MAS3_UW : 0);
> >         }
> > 
> > Here is a dump of the TLB info from Simics with the above 
> code present:
> > ------
> > L2 TLB1
> >                                             GT              
>      SSS UUU V I
> >  Row  Logical           Physical            SS TLPID  TID  
> WIMGE XWR XWR F P   V
> > ----- ----------------- ------------------- -- ----- ----- 
> ----- --- --- - -   -
> >   0   c0000000-cfffffff 000000000-00fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> >   1   d0000000-dfffffff 010000000-01fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> >   2   e0000000-efffffff 020000000-02fffffff 00     0     0  
>  M   XWR XWR 0 1   1
> > 
> > Actually this conditional code was only used for two legacy 
> functions:
> > 
> >   1: support KGDB to set break point.
> >      KGDB already dropped this; now uses its core write to 
> set break point.
> > 
> >   2: io_block_mapping() to create TLB in segmentation size 
> (not PAGE_SIZE)
> >      for device IO space.
> >      This use case is also removed from the latest PowerPC kernel.
> 
> io_block_mapping() went away, but the feature itself is still 
> useful and might come back with something like this:
> 
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg3
3851.html
> 
> ...though I'm not sure why such mappings would ever have user access.
> 
> This could end up being used for large user pages by 
> something like hugetlbfs or KVM, though.  I don't think we 
> want to make large user pages fail, especailly if it just 

Understand. 

Actually the following is my original modification. 
======
+#if defined(CONFIG_FSL_BOOKE) && defined(CONFIG_PTE_64BIT)
+	/* On there _PAGE_BAP_UR is always integrated into flag,
_PAGE_KERNEL_RWX 
+	 * and _PAGE_USER here. So we have to only check _PAGE_BAP_UR as
the condition.
+ 	 */
+	if (flags & _PAGE_BAP_UR) {
+#else
	if (flags & _PAGE_USER) {
+#endif

But I find there is no any usage for this, except for the above #1> KGDB
and #2 io_block_mapping(). So I think it's possible to remove this
completely :)

> happens with the 32-bit page table format (which i may not 
> what the person adding such a feature tests with).
> 
> I don't see a generic accessor that can test PTE flags for 
> user access -- in the absence of one, I guess we need an 
> ifdef here.  Or at least put in a comment so anyone who adds 
> a userspace use knows they need to fix it.

I already notice Ben's advice and looks fine to us.

Tiejun

> 
> -Scott
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>