[v3,6/7] powerpc: kprobes: emulate instructions on kprobe handler re-entry

Message ID 0d4193a90e4b2ad3cd2ef33d41228b80a2ac9f91.1492604782.git.naveen.n.rao@linux.vnet.ibm.com
State Accepted
Commit 22d8b3dec214cd43a773f621f95d254c50d2a092
Headers show

Commit Message

Naveen N. Rao April 19, 2017, 12:51 p.m.
On kprobe handler re-entry, try to emulate the instruction rather than
single stepping always.

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Masami Hiramatsu April 19, 2017, 2:43 p.m. | #1
BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
makes meaningful change.

Thank you,

On Wed, 19 Apr 2017 18:21:05 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/kprobes.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 46e8c1e03ce4..067e9863bfdf 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -276,6 +276,14 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>  			kprobes_inc_nmissed_count(p);
>  			prepare_singlestep(p, regs);
>  			kcb->kprobe_status = KPROBE_REENTER;
> +			if (p->ainsn.boostable >= 0) {
> +				ret = try_to_emulate(p, regs);
> +
> +				if (ret > 0) {
> +					restore_previous_kprobe(kcb);
> +					return 1;
> +				}
> +			}
>  			return 1;
>  		} else {
>  			if (*addr != BREAKPOINT_INSTRUCTION) {
> -- 
> 2.12.1
>
Naveen N. Rao April 19, 2017, 4:42 p.m. | #2
Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
> 
> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
> makes meaningful change.

Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
patch series.

Since this has to go through the powerpc tree, I followed this since I 
felt that Michael Ellerman prefers to keep functional changes separate 
from refactoring. I'm fine with either approach.

Michael?

Thanks!
- Naveen

> 
> Thank you,
> 
> On Wed, 19 Apr 2017 18:21:05 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 
>> On kprobe handler re-entry, try to emulate the instruction rather than
>> single stepping always.
>> 
>> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/kprobes.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>> index 46e8c1e03ce4..067e9863bfdf 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -276,6 +276,14 @@ int __kprobes kprobe_handler(struct pt_regs *regs)
>>  			kprobes_inc_nmissed_count(p);
>>  			prepare_singlestep(p, regs);
>>  			kcb->kprobe_status = KPROBE_REENTER;
>> +			if (p->ainsn.boostable >= 0) {
>> +				ret = try_to_emulate(p, regs);
>> +
>> +				if (ret > 0) {
>> +					restore_previous_kprobe(kcb);
>> +					return 1;
>> +				}
>> +			}
>>  			return 1;
>>  		} else {
>>  			if (*addr != BREAKPOINT_INSTRUCTION) {
>> -- 
>> 2.12.1
>> 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
> 
>
Michael Ellerman April 20, 2017, 6:11 a.m. | #3
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
>> 
>> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
>> makes meaningful change.
>
> Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
> patch series.
>
> Since this has to go through the powerpc tree, I followed this since I 
> felt that Michael Ellerman prefers to keep functional changes separate 
> from refactoring. I'm fine with either approach.
>
> Michael?

Yeah I'd definitely like this to be two patches. Otherwise when reading
the combined diff it's much harder to see the change.

cheers
Masami Hiramatsu April 21, 2017, 1:48 p.m. | #4
On Thu, 20 Apr 2017 16:11:10 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > Excerpts from Masami Hiramatsu's message of April 19, 2017 20:13:
> >> 
> >> BTW, as I pointed, 5/7 and 6/7 should be merged since this actually
> >> makes meaningful change.
> >
> > Yes, sorry if I wasn't clear in my previous reply in the (!) previous 
> > patch series.
> >
> > Since this has to go through the powerpc tree, I followed this since I 
> > felt that Michael Ellerman prefers to keep functional changes separate 
> > from refactoring. I'm fine with either approach.
> >
> > Michael?
> 
> Yeah I'd definitely like this to be two patches. Otherwise when reading
> the combined diff it's much harder to see the change.

OK, let it be so.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,
Michael Ellerman April 24, 2017, 10:47 p.m. | #5
On Wed, 2017-04-19 at 12:51:05 UTC, "Naveen N. Rao" wrote:
> On kprobe handler re-entry, try to emulate the instruction rather than
> single stepping always.
> 
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/22d8b3dec214cd43a773f621f95d25

cheers

Patch

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 46e8c1e03ce4..067e9863bfdf 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -276,6 +276,14 @@  int __kprobes kprobe_handler(struct pt_regs *regs)
 			kprobes_inc_nmissed_count(p);
 			prepare_singlestep(p, regs);
 			kcb->kprobe_status = KPROBE_REENTER;
+			if (p->ainsn.boostable >= 0) {
+				ret = try_to_emulate(p, regs);
+
+				if (ret > 0) {
+					restore_previous_kprobe(kcb);
+					return 1;
+				}
+			}
 			return 1;
 		} else {
 			if (*addr != BREAKPOINT_INSTRUCTION) {