| Message ID | 20200501034220.8982-15-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (54dc28ff5e0b3585224d49a31b53e030342ca5c3) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 104 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
> @@ -524,7 +524,10 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned > long addr) struct module *mod = rec->arch.mod; > > /* read where this goes */ > - if (probe_kernel_read(op, ip, sizeof(op))) > + if (probe_kernel_read_inst(op, ip)) > + return -EFAULT; > + > + if (probe_kernel_read_inst(op + 1, ip + 4)) > return -EFAULT; I had to double check the above for what happens when we introduce prefix instructions but it looks mostly correct. There does however look to be a corner case that could alter the behaviour once prefix instructions are introduced. With prefix instructions probe_kernel_read_inst() will read 8 bytes if the first 4 bytes are a valid prefix. Therefore the above could end up trying to read 12 bytes in total if the ip is a normal instruction and ip+4 is prefixed. Obviously this is never going to match the expected nop sequence, and prefixed instructions shouldn't cross page boundaries so the extra 4 bytes should never be the cause of a fault either. The only difference we might see is ftrace_make_call() incorrectly returning -EFAULT instead of -EINVAL for an invalid (ie. crossing a 64 byte boundary) prefix instruction sequence. In practice this doesn't seem like it would cause any real issues and the rest of the patch does not appear to change any existing behaviour. Reviewed-by: Alistair Popple <alistair@popple.id.au> > > if (!expected_nop_sequence(ip, op[0], op[1])) { > @@ -587,7 +590,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long > addr) unsigned long ip = rec->ip; > > /* read where this goes */ > - if (probe_kernel_read(&op, (void *)ip, MCOUNT_INSN_SIZE)) > + if (probe_kernel_read_inst(&op, (void *)ip)) > return -EFAULT; > > /* It should be pointing to a nop */ > @@ -643,7 +646,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace > *rec, unsigned long addr) } > > /* Make sure we have a nop */ > - if (probe_kernel_read(&op, ip, sizeof(op))) { > + if (probe_kernel_read_inst(&op, ip)) { > pr_err("Unable to read ftrace location %p\n", ip); > return -EFAULT; > } > @@ -721,7 +724,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned > long old_addr, } > > /* read where this goes */ > - if (probe_kernel_read(&op, (void *)ip, sizeof(int))) { > + if (probe_kernel_read_inst(&op, (void *)ip)) { > pr_err("Fetching opcode failed.\n"); > return -EFAULT; > } > diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c > index eaf786afad2b..08dedd927268 100644 > --- a/arch/powerpc/lib/inst.c > +++ b/arch/powerpc/lib/inst.c > @@ -16,3 +16,14 @@ int probe_user_read_inst(struct ppc_inst *inst, > *inst = ppc_inst(val); > return err; > } > + > +int probe_kernel_read_inst(struct ppc_inst *inst, > + struct ppc_inst *src) > +{ > + unsigned int val; > + int err; > + > + err = probe_kernel_read(&val, src, sizeof(val)); > + *inst = ppc_inst(val); > + return err; > +}
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h index 3e9a58420151..0d581b332c20 100644 --- a/arch/powerpc/include/asm/inst.h +++ b/arch/powerpc/include/asm/inst.h @@ -39,5 +39,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y) int probe_user_read_inst(struct ppc_inst *inst, struct ppc_inst *nip); +int probe_kernel_read_inst(struct ppc_inst *inst, + struct ppc_inst *src); #endif /* _ASM_INST_H */ diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c index 6f49971a3cd4..0ad2c9d4ab49 100644 --- a/arch/powerpc/kernel/trace/ftrace.c +++ b/arch/powerpc/kernel/trace/ftrace.c @@ -68,7 +68,7 @@ ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new) */ /* read the text we want to modify */ - if (probe_kernel_read(&replaced, (void *)ip, MCOUNT_INSN_SIZE)) + if (probe_kernel_read_inst(&replaced, (void *)ip)) return -EFAULT; /* Make sure it is what we expect it to be */ @@ -130,7 +130,7 @@ __ftrace_make_nop(struct module *mod, struct ppc_inst op, pop; /* read where this goes */ - if (probe_kernel_read(&op, (void *)ip, sizeof(int))) { + if (probe_kernel_read_inst(&op, (void *)ip)) { pr_err("Fetching opcode failed.\n"); return -EFAULT; } @@ -164,7 +164,7 @@ __ftrace_make_nop(struct module *mod, /* When using -mkernel_profile there is no load to jump over */ pop = ppc_inst(PPC_INST_NOP); - if (probe_kernel_read(&op, (void *)(ip - 4), 4)) { + if (probe_kernel_read_inst(&op, (void *)(ip - 4))) { pr_err("Fetching instruction at %lx failed.\n", ip - 4); return -EFAULT; } @@ -196,7 +196,7 @@ __ftrace_make_nop(struct module *mod, * Check what is in the next instruction. We can see ld r2,40(r1), but * on first pass after boot we will see mflr r0. */ - if (probe_kernel_read(&op, (void *)(ip+4), MCOUNT_INSN_SIZE)) { + if (probe_kernel_read_inst(&op, (void *)(ip+4))) { pr_err("Fetching op failed.\n"); return -EFAULT; } @@ -348,7 +348,7 @@ static int setup_mcount_compiler_tramp(unsigned long tramp) return -1; /* New trampoline -- read where this goes */ - if (probe_kernel_read(&op, (void *)tramp, sizeof(int))) { + if (probe_kernel_read_inst(&op, (void *)tramp)) { pr_debug("Fetching opcode failed.\n"); return -1; } @@ -398,7 +398,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr) struct ppc_inst op; /* Read where this goes */ - if (probe_kernel_read(&op, (void *)ip, sizeof(int))) { + if (probe_kernel_read_inst(&op, (void *)ip)) { pr_err("Fetching opcode failed.\n"); return -EFAULT; } @@ -524,7 +524,10 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) struct module *mod = rec->arch.mod; /* read where this goes */ - if (probe_kernel_read(op, ip, sizeof(op))) + if (probe_kernel_read_inst(op, ip)) + return -EFAULT; + + if (probe_kernel_read_inst(op + 1, ip + 4)) return -EFAULT; if (!expected_nop_sequence(ip, op[0], op[1])) { @@ -587,7 +590,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) unsigned long ip = rec->ip; /* read where this goes */ - if (probe_kernel_read(&op, (void *)ip, MCOUNT_INSN_SIZE)) + if (probe_kernel_read_inst(&op, (void *)ip)) return -EFAULT; /* It should be pointing to a nop */ @@ -643,7 +646,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr) } /* Make sure we have a nop */ - if (probe_kernel_read(&op, ip, sizeof(op))) { + if (probe_kernel_read_inst(&op, ip)) { pr_err("Unable to read ftrace location %p\n", ip); return -EFAULT; } @@ -721,7 +724,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr, } /* read where this goes */ - if (probe_kernel_read(&op, (void *)ip, sizeof(int))) { + if (probe_kernel_read_inst(&op, (void *)ip)) { pr_err("Fetching opcode failed.\n"); return -EFAULT; } diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c index eaf786afad2b..08dedd927268 100644 --- a/arch/powerpc/lib/inst.c +++ b/arch/powerpc/lib/inst.c @@ -16,3 +16,14 @@ int probe_user_read_inst(struct ppc_inst *inst, *inst = ppc_inst(val); return err; } + +int probe_kernel_read_inst(struct ppc_inst *inst, + struct ppc_inst *src) +{ + unsigned int val; + int err; + + err = probe_kernel_read(&val, src, sizeof(val)); + *inst = ppc_inst(val); + return err; +}
Introduce a probe_kernel_read_inst() function to use in cases where probe_kernel_read() is used for getting an instruction. This will be more useful for prefixed instructions. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v6: - This was previously just in ftrace.c --- arch/powerpc/include/asm/inst.h | 2 ++ arch/powerpc/kernel/trace/ftrace.c | 23 +++++++++++++---------- arch/powerpc/lib/inst.c | 11 +++++++++++ 3 files changed, 26 insertions(+), 10 deletions(-)