Excerpts from Michael Ellerman's message of May 22, 2020 11:33 pm: > In a few places we want to calculate the address of the next > instruction. Previously that was simple, we just added 4 bytes, or if > using a u32 * we incremented that pointer by 1. > > But prefixed instructions make it more complicated, we need to advance > by either 4 or 8 bytes depending on the actual instruction. We also > can't do pointer arithmetic using struct ppc_inst, because it is > always 8 bytes in size on 64-bit, even though we might only need to > advance by 4 bytes. > > So add a ppc_inst_next() helper which calculates the location of the > next instruction, if the given instruction was located at the given > address. Note the instruction doesn't need to actually be at the > address in memory. > > Although it would seem natural for the value to be passed by value, > that makes it too easy to write a loop that will read off the end of a > page, eg: > > for (; src < end; src = ppc_inst_next(src, *src), > dest = ppc_inst_next(dest, *dest)) > > As noticed by Christophe and Jordan, if end is the exact end of a > page, and the next page is not mapped, this will fault, because *dest > will read 8 bytes, 4 bytes into the next page. > > So value is passed by reference, so the helper can be careful to use > ppc_inst_read() on it. > > Signed-off-by: Michael Ellerman <m...@ellerman.id.au> > --- > arch/powerpc/include/asm/inst.h | 13 +++++++++++++ > arch/powerpc/kernel/uprobes.c | 2 +- > arch/powerpc/lib/feature-fixups.c | 15 ++++++++------- > arch/powerpc/xmon/xmon.c | 2 +- > 4 files changed, 23 insertions(+), 9 deletions(-) > > v2: Pass the value as a pointer and use ppc_inst_read() on it. > > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > index d82e0c99cfa1..5b756ba77ed2 100644 > --- a/arch/powerpc/include/asm/inst.h > +++ b/arch/powerpc/include/asm/inst.h > @@ -100,6 +100,19 @@ static inline int ppc_inst_len(struct ppc_inst x) > return ppc_inst_prefixed(x) ? 8 : 4; > } > > +/* > + * Return the address of the next instruction, if the instruction @value was > + * located at @location. > + */ > +static inline struct ppc_inst *ppc_inst_next(void *location, struct ppc_inst > *value) > +{ > + struct ppc_inst tmp; > + > + tmp = ppc_inst_read(value); > + > + return location + ppc_inst_len(tmp); > +} > + > int probe_user_read_inst(struct ppc_inst *inst, > struct ppc_inst __user *nip); > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index 83e883e1a42d..d200e7df7167 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -112,7 +112,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, > struct pt_regs *regs) > * support doesn't exist and have to fix-up the next instruction > * to be executed. > */ > - regs->nip = utask->vaddr + ppc_inst_len(ppc_inst_read(&auprobe->insn)); > + regs->nip = (unsigned long)ppc_inst_next((void *)utask->vaddr, > &auprobe->insn); > > user_disable_single_step(current); > return 0;
AFAIKS except for here, there is no need for the void *location arg. I would prefer to drop that and keep this unchanged (it's a slightly special case anyway). Thanks, Nick > diff --git a/arch/powerpc/lib/feature-fixups.c > b/arch/powerpc/lib/feature-fixups.c > index 80f320c2e189..4c0a7ee9fa00 100644 > --- a/arch/powerpc/lib/feature-fixups.c > +++ b/arch/powerpc/lib/feature-fixups.c > @@ -68,7 +68,7 @@ static int patch_alt_instruction(struct ppc_inst *src, > struct ppc_inst *dest, > > static int patch_feature_section(unsigned long value, struct fixup_entry > *fcur) > { > - struct ppc_inst *start, *end, *alt_start, *alt_end, *src, *dest; > + struct ppc_inst *start, *end, *alt_start, *alt_end, *src, *dest, nop; > > start = calc_addr(fcur, fcur->start_off); > end = calc_addr(fcur, fcur->end_off); > @@ -84,14 +84,15 @@ static int patch_feature_section(unsigned long value, > struct fixup_entry *fcur) > src = alt_start; > dest = start; > > - for (; src < alt_end; src = (void *)src + > ppc_inst_len(ppc_inst_read(src)), > - (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) { > + for (; src < alt_end; src = ppc_inst_next(src, src), > + dest = ppc_inst_next(dest, dest)) { > if (patch_alt_instruction(src, dest, alt_start, alt_end)) > return 1; > } > > - for (; dest < end; dest = (void *)dest + > ppc_inst_len(ppc_inst(PPC_INST_NOP))) > - raw_patch_instruction(dest, ppc_inst(PPC_INST_NOP)); > + nop = ppc_inst(PPC_INST_NOP); > + for (; dest < end; dest = ppc_inst_next(dest, &nop)) > + raw_patch_instruction(dest, nop); > > return 0; > } > @@ -405,8 +406,8 @@ static void do_final_fixups(void) > while (src < end) { > inst = ppc_inst_read(src); > raw_patch_instruction(dest, inst); > - src = (void *)src + ppc_inst_len(inst); > - dest = (void *)dest + ppc_inst_len(inst); > + src = ppc_inst_next(src, src); > + dest = ppc_inst_next(dest, dest); > } > #endif > } > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index fb135f2cd6b0..65cf853a4d26 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -939,7 +939,7 @@ static void insert_bpts(void) > } > > patch_instruction(bp->instr, instr); > - patch_instruction((void *)bp->instr + ppc_inst_len(instr), > + patch_instruction(ppc_inst_next(bp->instr, &instr), > ppc_inst(bpinstr)); > if (bp->enabled & BP_CIABR) > continue; > -- > 2.25.1 > >