[0/6] PowerPc 8xx TLB/MMU fixes

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 06/10/2009 03:52:15:
>
> \
> > So how does this look? Does it change anything?
> > It should as the previous way was way off :(
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index c33c6de..08a392f 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -153,7 +153,7 @@ int __kprobes do_page_fault(struct pt_regs *regs,
> unsigned long address,
> >  #ifdef DEBUG_DCBX
> >        const char *istr = NULL;
> >
> > -      insn = *((unsigned long *)regs->nip);
> > +      __get_user(insn, (unsigned long __user *)regs->nip);
>
> No, use get_user() not __get_user() or if you use the later, also use
> access_ok(), and test the result in case it errors (if it does, you
> probably want to just goto bad access and SEGV).

OK, lets see what this gives us:

> > No, use get_user() not __get_user() or if you use the later, also use
> > access_ok(), and test the result in case it errors (if it does, you
> > probably want to just goto bad access and SEGV).
> 
> OK, lets see what this gives us:

Hrm... did you change anything ? :-)

Ben.

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index c33c6de..1bf91d3 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  #ifdef DEBUG_DCBX
>  		const char *istr = NULL;
> 
> -		insn = *((unsigned long *)regs->nip);
> +		insn = 0;
> +		__get_user(insn, (unsigned long __user *)regs->nip);
>  		if (((insn >> (31-5)) & 0x3f) == 31) {
>  			if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */
>  				istr = "dcbz";
> @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  				dar = regs->gpr[rb];
>  				if (ra)
>  					dar += regs->gpr[ra];
> -				if (dar != address && address != 0x00f0 && trap == 0x300)
> +				if (dar != address && trap == 0x300)
>  					printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
>  				if (!strcmp(istr, "dcbst") && is_write) {
>  					printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
>  					       ra, rb, dar);
>  					is_write = 0;
>  				}
> -
> +#if 0
>  				if (trap == 0x300 && address != dar) {
>  					__asm__ ("mtdar %0" : : "r" (dar));
>  					return 0;
>  				}
> +#endif
>  			}
>  		}
>  #endif
>  		if (address == 0x00f0 && trap == 0x300) {
> -			pte_t *ptep;
> +			//pte_t *ptep;
> 
>  			/* This is from a dcbX or icbi insn gone bad, these
>  			 * insn do not set DAR so we have to do it here instead */
> -			insn = *((unsigned long *)regs->nip);
> +			if (get_user(insn, (unsigned long __user *)regs->nip)) {
> +				printk(KERN_CRIT "get_user failed, NIP:%lx\n",
> +				       regs->nip);
> +				goto bad_area_nosemaphore;
> +			}
> 
>  			ra = (insn >> (31-15)) & 0x1f; /* Reg RA */
>  			rb = (insn >> (31-20)) & 0x1f; /* Reg RB */
> @@ -206,7 +212,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  			       trap, address, dar, error_code, istr);
>  #endif
>  			address = dar;
> -#if 1
> +#if 0
>  			if (is_write && get_pteptr(mm, dar, &ptep, NULL)) {
>  				pte_t my_pte = *ptep;
> 
> @@ -216,7 +222,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  				}
>  			}
>  #else
> -			return 0;
> +			//return 0;
>  #endif
>  		}
>  	}

>
>
> > > No, use get_user() not __get_user() or if you use the later, also use
> > > access_ok(), and test the result in case it errors (if it does, you
> > > probably want to just goto bad access and SEGV).
> >
> > OK, lets see what this gives us:
>
> Hrm... did you change anything ? :-)

Yes, see below

>
> Ben.
>
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index c33c6de..1bf91d3 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs,
> unsigned long address,
> >  #ifdef DEBUG_DCBX
> >        const char *istr = NULL;
> >
> > -      insn = *((unsigned long *)regs->nip);
> > +      insn = 0;
> > +      __get_user(insn, (unsigned long __user *)regs->nip);

Here I don't care if err. insn will be 0 if it fails and the following
if will be false

> >        if (((insn >> (31-5)) & 0x3f) == 31) {
> >           if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */
> >              istr = "dcbz";
> > @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs,
> unsigned long address,
> >              dar = regs->gpr[rb];
> >              if (ra)
> >                 dar += regs->gpr[ra];
> > -            if (dar != address && address != 0x00f0 && trap == 0x300)
> > +            if (dar != address && trap == 0x300)
> >                 printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
> >              if (!strcmp(istr, "dcbst") && is_write) {
> >                 printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
> >                        ra, rb, dar);
> >                 is_write = 0;
> >              }
> > -
> > +#if 0
> >              if (trap == 0x300 && address != dar) {
> >                 __asm__ ("mtdar %0" : : "r" (dar));
> >                 return 0;
> >              }
> > +#endif
> >           }
> >        }
> >  #endif
> >        if (address == 0x00f0 && trap == 0x300) {
> > -         pte_t *ptep;
> > +         //pte_t *ptep;
> >
> >           /* This is from a dcbX or icbi insn gone bad, these
> >            * insn do not set DAR so we have to do it here instead */
> > -         insn = *((unsigned long *)regs->nip);
> > +         if (get_user(insn, (unsigned long __user *)regs->nip)) {
> > +            printk(KERN_CRIT "get_user failed, NIP:%lx\n",
> > +                   regs->nip);
> > +            goto bad_area_nosemaphore;
> > +         }

and here I go to bad_area

On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote:

> Here I don't care if err. insn will be 0 if it fails and the following
> if will be false

I'd rather you use get_user() so it does access_ok().

Else, you can probably manufacture some code that will make the kernel
access some MMIO register for example, which could be nasty.

At this point, you may as well also check the result even if indeed a
fault isn't going to matter. Just makes the code cleaner and avoids some
random janitor coming up with a patch later on :-)

Cheers,
Ben.

> > >        if (((insn >> (31-5)) & 0x3f) == 31) {
> > >           if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */
> > >              istr = "dcbz";
> > > @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs,
> > unsigned long address,
> > >              dar = regs->gpr[rb];
> > >              if (ra)
> > >                 dar += regs->gpr[ra];
> > > -            if (dar != address && address != 0x00f0 && trap == 0x300)
> > > +            if (dar != address && trap == 0x300)
> > >                 printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar);
> > >              if (!strcmp(istr, "dcbst") && is_write) {
> > >                 printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n",
> > >                        ra, rb, dar);
> > >                 is_write = 0;
> > >              }
> > > -
> > > +#if 0
> > >              if (trap == 0x300 && address != dar) {
> > >                 __asm__ ("mtdar %0" : : "r" (dar));
> > >                 return 0;
> > >              }
> > > +#endif
> > >           }
> > >        }
> > >  #endif
> > >        if (address == 0x00f0 && trap == 0x300) {
> > > -         pte_t *ptep;
> > > +         //pte_t *ptep;
> > >
> > >           /* This is from a dcbX or icbi insn gone bad, these
> > >            * insn do not set DAR so we have to do it here instead */
> > > -         insn = *((unsigned long *)regs->nip);
> > > +         if (get_user(insn, (unsigned long __user *)regs->nip)) {
> > > +            printk(KERN_CRIT "get_user failed, NIP:%lx\n",
> > > +                   regs->nip);
> > > +            goto bad_area_nosemaphore;
> > > +         }
> 
> and here I go to bad_area

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 06/10/2009 13:06:26:

> From:
>
> Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> To:
>
> Joakim Tjernlund <joakim.tjernlund@transmode.se>
>
> Cc:
>
> "linuxppc-dev@ozlabs.org" <linuxppc-dev@ozlabs.org>, Rex Feany
> <RFeany@mrv.com>, Scott Wood <scottwood@freescale.com>
>
> Date:
>
> 06/10/2009 13:06
>
> Subject:
>
> Re: [PATCH 0/6] PowerPc 8xx TLB/MMU fixes
>
> On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote:
>
> > Here I don't care if err. insn will be 0 if it fails and the following
> > if will be false
>
> I'd rather you use get_user() so it does access_ok().

It is only debug code so it will go away
The real access is later.

I need do kernel space separately, is there a better way than:

if (regs->nip & 0xc0000000) /* kernel space ? */
	insn = *((unsigned long *)regs->nip);
else if (get_user(insn, (unsigned long *)regs->nip)) {
	printk(KERN_CRIT "get_user! NIP:%lx\n", regs->nip); /* DEBUG */
	goto bad_area_nosemaphore;
}