ARM: kprobes: only patch instructions on one CPU

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 <[email protected]>

Cc Nicolas Pitre <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---
 arch/arm/kernel/kprobes.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/kernel/kprobes.c +2 -2

index 129c116..e9f95300 100644
@@ -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)

Quoted 1 line · Rabin Vincent

The text patching needs to be done only once, instead of once on

each

Quoted 1 line

CPU. The other CPUs will busy wait inside the stop machine code

until

Quoted 1 line

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.

Quoted 14 lines

Cc Jon Medhurst <[email protected]>

Cc Nicolas Pitre <[email protected]>

Signed-off-by Rabin Vincent <[email protected]>

---
 arch/arm/kernel/kprobes.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/kernel/kprobes.c +0 -0

index 129c116..e9f95300 100644

arch/arm/kernel/kprobes.c void __kprobes arch_arm_kprobe(struct kprobe *p) · line 129

Quoted 3 lines

arch/arm/kernel/kprobes.c +0 -0

@@ -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 */

arch/arm/kernel/kprobes.c void __kprobes arch_arm_kprobe(struct kprobe *p) · line 130

Quoted 1 line

arch/arm/kernel/kprobes.c +0 -1

-             stop_machine(set_t32_breakpoint, (void *)addr,

&cpu_online_map);

arch/arm/kernel/kprobes.c void __kprobes arch_arm_kprobe(struct kprobe *p)

Quoted 3 lines

arch/arm/kernel/kprobes.c +1 -0

+             stop_machine(set_t32_breakpoint, (void *)addr, NULL);
      } else {
              /* Word aligned 32-bit instruction can be written

atomically */

Quoted 10 lines

arch/arm/kernel/kprobes.c +1 -1

              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)

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.)

Quoted 7 lines · Tixy

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.