Re: [git pull] x86 fixes

Harvey Harrison
2008-03-27T20:49:27Z

On Thu, 2008-03-27 at 13:31 -0700, Linus Torvalds wrote:
>
> On Thu, 27 Mar 2008, Ingo Molnar wrote:
> >
> > Ingo Molnar (1):
> > x86: fix prefetch workaround
> ...
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index fdc6674..c0c82bc 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -92,7 +92,8 @@ static int is_prefetch(struct pt_regs *regs, unsigned long addr,
> > unsigned char *max_instr;
> >
> > #ifdef CONFIG_X86_32
> > - if (!(__supported_pte_mask & _PAGE_NX))
> > + /* Catch an obscure case of prefetch inside an NX page: */
> > + if ((__supported_pte_mask & _PAGE_NX) && (error_code & 16))
> > return 0;
> > #endif
>
> Ingo, this patch makes no sense.
>
> Two reasons:
>
> - "error_code & 16" is senseless. Use PF_INSTR instead, which actually
> tells the reader something.
>
> - this piece of crap code is immediately followed by
>
> /* If it was a exec fault on NX page, ignore */
> if (error_code & PF_INSTR)
> return 0;
>
> which uses that *right* and readable PF_INSTR #define, and also shows
> that the newly modified code is totally insane (ie: if the new code
> triggers, then it would have returned 0 later _anyway_)
>
> So I think it's just crap. I think it's duplication from the merging of
> the x86 code, and I think that the fact that the new code didn't use the
> right #define helper means that people didn't see that it was crap.
>
> I pulled it, but this needs some resolution. The code makes no sense.
> Should that #ifdef and the code inside of it just be removed entirely?
>

Sorry, this was my fault, I eliminated the ifdef within the X86_32 block
but didn't go further. Think the below is correct:

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index c0c82bc..6f5df93 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -91,14 +91,8 @@ static int is_prefetch(struct pt_regs *regs, unsigned long addr,
int prefetch = 0;
unsigned char *max_instr;

-#ifdef CONFIG_X86_32
/* Catch an obscure case of prefetch inside an NX page: */
- if ((__supported_pte_mask & _PAGE_NX) && (error_code & 16))
- return 0;
-#endif
-
- /* If it was a exec fault on NX page, ignore */
- if (error_code & PF_INSTR)
+ if ((__supported_pte_mask & _PAGE_NX) && (error_code & PF_INSTR))
return 0;

instr = (unsigned char *)convert_ip_to_linear(current, regs);