[PATCH] ARM: kprobes: only patch instructions on one CPU

[PATCH] ARM: kprobes: only patch instructions on one CPU

[Rabin Vincent]

The text patching needs to be done only once, instead of once on each
CPU.  The other CPUs will busy wait inside the stop machine code until
the patching is done.

Cc: Jon Medhurst <tixy@yxit.co.uk>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/kprobes.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 129c116..e9f95300 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -127,7 +127,7 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
 		flush_insns(addr, sizeof(u16));
 	} else if (addr & 2) {
 		/* A 32-bit instruction spanning two words needs special care */
-		stop_machine(set_t32_breakpoint, (void *)addr, &cpu_online_map);
+		stop_machine(set_t32_breakpoint, (void *)addr, NULL);
 	} else {
 		/* Word aligned 32-bit instruction can be written atomically */
 		u32 bkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
@@ -190,7 +190,7 @@ int __kprobes __arch_disarm_kprobe(void *p)
 
 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
-	stop_machine(__arch_disarm_kprobe, p, &cpu_online_map);
+	stop_machine(__arch_disarm_kprobe, p, NULL);
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
-- 
1.7.5.4

[PATCH] ARM: kprobes: only patch instructions on one CPU

[Tixy]

On Wed, 2011-10-12 at 21:42 +0530, Rabin Vincent wrote:
> The text patching needs to be done only once, instead of once on each
> CPU.  The other CPUs will busy wait inside the stop machine code until
> the patching is done.

Where in the stop machine code do the other CPU's busy wait?

How I read the code is that __stop_cpus() calls queue_stop_cpus_work()
which queues the work on each of the specified CPU's work list and wakes
that CPU's stopper thread to process it. __stop_cpus() then calls
wait_for_completion() to wait for these CPUs to finish the work.

I don't see how the execution path of CPUs not specified in cpumask is
interrupted in any way.

-- 
Tixy

> 
> Cc: Jon Medhurst <tixy@yxit.co.uk>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/kernel/kprobes.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> index 129c116..e9f95300 100644
> --- a/arch/arm/kernel/kprobes.c
> +++ b/arch/arm/kernel/kprobes.c
> @@ -127,7 +127,7 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
>  		flush_insns(addr, sizeof(u16));
>  	} else if (addr & 2) {
>  		/* A 32-bit instruction spanning two words needs special care */
> -		stop_machine(set_t32_breakpoint, (void *)addr, &cpu_online_map);
> +		stop_machine(set_t32_breakpoint, (void *)addr, NULL);
>  	} else {
>  		/* Word aligned 32-bit instruction can be written atomically */
>  		u32 bkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
> @@ -190,7 +190,7 @@ int __kprobes __arch_disarm_kprobe(void *p)
>  
>  void __kprobes arch_disarm_kprobe(struct kprobe *p)
>  {
> -	stop_machine(__arch_disarm_kprobe, p, &cpu_online_map);
> +	stop_machine(__arch_disarm_kprobe, p, NULL);
>  }
>  
>  void __kprobes arch_remove_kprobe(struct kprobe *p)

[PATCH] ARM: kprobes: only patch instructions on one CPU

[Tixy]

On Thu, 2011-10-13 at 08:55 +0100, Tixy wrote:
> On Wed, 2011-10-12 at 21:42 +0530, Rabin Vincent wrote:
> > The text patching needs to be done only once, instead of once on
> each
> > CPU.  The other CPUs will busy wait inside the stop machine code
> until
> > the patching is done.
> 
> Where in the stop machine code do the other CPU's busy wait?
> 
> How I read the code is that __stop_cpus() calls queue_stop_cpus_work()
> which queues the work on each of the specified CPU's work list and
> wakes
> that CPU's stopper thread to process it. __stop_cpus() then calls
> wait_for_completion() to wait for these CPUs to finish the work.
> 
> I don't see how the execution path of CPUs not specified in cpumask is
> interrupted in any way.
> 

As I said in my reply to the other mail, I had missed the fact that it
is stop_machine_cpu_stop() which is used to call our function, and this
synchronises all cores. Therefore, this patch is correct, assuming that
a flush_icache_range executed on one core also flushes I-caches on other
cores. (I'm a bit doubtfull of this as I beleive that at least
ARM11MPCore requires this to be managed in software and I can't find any
code that handles this.)

-- 
Tixy

> 
> > 
> > Cc: Jon Medhurst <tixy@yxit.co.uk>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Signed-off-by: Rabin Vincent <rabin@rab.in>
> > ---
> >  arch/arm/kernel/kprobes.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
> > index 129c116..e9f95300 100644
> > --- a/arch/arm/kernel/kprobes.c
> > +++ b/arch/arm/kernel/kprobes.c
> > @@ -127,7 +127,7 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
> >               flush_insns(addr, sizeof(u16));
> >       } else if (addr & 2) {
> >               /* A 32-bit instruction spanning two words needs
> special care */
> > -             stop_machine(set_t32_breakpoint, (void *)addr,
> &cpu_online_map);
> > +             stop_machine(set_t32_breakpoint, (void *)addr, NULL);
> >       } else {
> >               /* Word aligned 32-bit instruction can be written
> atomically */
> >               u32 bkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
> > @@ -190,7 +190,7 @@ int __kprobes __arch_disarm_kprobe(void *p)
> >  
> >  void __kprobes arch_disarm_kprobe(struct kprobe *p)
> >  {
> > -     stop_machine(__arch_disarm_kprobe, p, &cpu_online_map);
> > +     stop_machine(__arch_disarm_kprobe, p, NULL);
> >  }
> >  
> >  void __kprobes arch_remove_kprobe(struct kprobe *p)
> 
> 
> 

[PATCH] ARM: kprobes: only patch instructions on one CPU

[Rabin Vincent]

On Thu, Oct 13, 2011 at 10:07:32AM +0100, Tixy wrote:
> As I said in my reply to the other mail, I had missed the fact that it
> is stop_machine_cpu_stop() which is used to call our function, and this
> synchronises all cores. Therefore, this patch is correct, assuming that
> a flush_icache_range executed on one core also flushes I-caches on other
> cores. (I'm a bit doubtfull of this as I beleive that at least
> ARM11MPCore requires this to be managed in software and I can't find any
> code that handles this.)

If we want kprobes to work correctly on ARM11MPCore, shouldn't we need
to broadcast the invalidate in arch_prepare_kprobe() and the
non-stop-machine-using parts of arch_arm_kprobe() too?

Also, it seems odd to perform the instruction write and
flush_icache_range() on every CPU from stop_machine().  Perhaps it would
be better if there were a way in stop_machine() to call an I-cache
invalidation function on all CPUs, after the arm/disarm function is
called on only one of them.

[PATCH] ARM: kprobes: only patch instructions on one CPU

[Tixy]

On Tue, 2011-10-25 at 21:06 +0530, Rabin Vincent wrote:
> On Thu, Oct 13, 2011 at 10:07:32AM +0100, Tixy wrote:
> > As I said in my reply to the other mail, I had missed the fact that it
> > is stop_machine_cpu_stop() which is used to call our function, and this
> > synchronises all cores. Therefore, this patch is correct, assuming that
> > a flush_icache_range executed on one core also flushes I-caches on other
> > cores. (I'm a bit doubtfull of this as I beleive that at least
> > ARM11MPCore requires this to be managed in software and I can't find any
> > code that handles this.)
> 
> If we want kprobes to work correctly on ARM11MPCore, shouldn't we need
> to broadcast the invalidate in arch_prepare_kprobe() and the
> non-stop-machine-using parts of arch_arm_kprobe() too?

We would, though the effect of not doing so would be safer. I.e. we
would just miss triggering kprobes, whereas when removing probes the
effect could be a processes faulting when it executes the 'breakpoint'
instruction still in the I cache.

> Also, it seems odd to perform the instruction write and
> flush_icache_range() on every CPU from stop_machine().  Perhaps it would
> be better if there were a way in stop_machine() to call an I-cache
> invalidation function on all CPUs, after the arm/disarm function is
> called on only one of them.

The arm/disarm functions only writes an instruction to RAM and flushes
the I-cache for it. It doesn't seem worth the effort of adding code to
avoid the couple of instruction required to write the instruction,
especially as stop_machine() is already causing all CPUs to reschedule,
sync with each other, and reschedule again.

As you say though, it should be possible and looks nicer if the disarm
code was only called on one CPU. I'm just trying to point out that there
may be risks and there is little performance benefit.

-- 
Tixy