diff mbox series

[v2] powerpc: Add ppc_inst_next()

Message ID 20200522133318.1681406-1-mpe@ellerman.id.au
State Under Review
Headers show
Series [v2] powerpc: Add ppc_inst_next() | expand

Checks

Context Check Description
snowpatch_ozlabs/needsstable success Patch has no Fixes tags
snowpatch_ozlabs/checkpatch warning Test checkpatch on branch powerpc/merge
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (554840f2574c18f341dcfb4144b280b4842186fe)

Commit Message

Michael Ellerman May 22, 2020, 1:33 p.m. UTC
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@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.

Comments

Nicholas Piggin May 23, 2020, 11:56 p.m. UTC | #1
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@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
> 
>
Nicholas Piggin May 24, 2020, 12:01 a.m. UTC | #2
Excerpts from Nicholas Piggin's message of May 24, 2020 9:56 am:
> 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@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).

Ooops, I didn't read the last thread. I don't think insert_bpts needs it 
though, only this case. Anyway it's a minor nitpick so if it's already 
been considered, fine.

Thanks,
Nick
Michael Ellerman May 25, 2020, 2:41 a.m. UTC | #3
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Nicholas Piggin's message of May 24, 2020 9:56 am:
>> 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@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).
>
> Ooops, I didn't read the last thread. I don't think insert_bpts needs it 
> though, only this case. Anyway it's a minor nitpick so if it's already 
> been considered, fine.

There's a few places that don't need it, eg:

+       nop = ppc_inst(PPC_INST_NOP);
+       for (; dest < end; dest = ppc_inst_next(dest, &nop))
+               raw_patch_instruction(dest, nop);


But I prefer the way that reads, it's clear we're incrementing by the
length of a nop, even though we could read the nop from dest because we
just patched it.

If we ever did execute-only kernel text, it would help to have the
location and value separate too, because then reading from the text
would require a helper, but reading from data/stack would not.

So I'll go with this version.

cheers
Jordan Niethe May 25, 2020, 3:28 a.m. UTC | #4
On Fri, May 22, 2020 at 11:33 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> 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@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;
> 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
>
Reviewed-by: Jordan Niethe <jniethe5@gmail.com>
diff mbox series

Patch

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;
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;