[PATCH v2] powerpc: Add ppc_inst_next()

[Nicholas Piggin]

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 <mpe at 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
> 
>