[PATCH] [RFC PATCH] ARM: kprobes: Make breakpoint setting/clearing SMP safe

[PATCH] [RFC PATCH] ARM: kprobes: Make breakpoint setting/clearing SMP safe

[Tixy]

From: Jon Medhurst <tixy@yxit.co.uk>

On SMP systems, kprobes has two different issues when manipulating
breakpoints.

When disarming probes, with two CPUs (A and B) we can get:

- A takes undef exception due to hitting a probe breakpoint.
- B disarms probe, replacing the breakpoint with original instruction.
- A's undef handler reads instruction at PC, doesn't see kprobe
  breakpoint, therefore doesn't call kprobe_handler and the process
  takes a fatal exception.

The second issue occurs when both arming and disarming probes on a
32-bit Thumb instruction which straddles two memory words. In this case
it's possible that another CPU will read 16 bits from the new
instruction and 16 from the old.

Both these issues are known and the kprobe implementation uses
stop_machine() to avoid them, however, this is not sufficient.
stop_machine() does not perform any kind on synchronisation between CPUs
so it it still possible for one CPU to call the breakpoint changing
function before another CPU has been interrupted to do likewise.

To fix this problem, this patch creates a new function
sync_stop_machine() which ensures that all online CPUs execute the
specified function at the same time.

I am looking for feedback:
- Have I got my understanding of stop_machine() right?
- Are there other APIs to do what we need?
- Is my patch robust.
- Do I have the correct barriers?

Signed-off-by: Jon Medhurst <tixy@yxit.co.uk>
---
 arch/arm/kernel/kprobes.c |   55 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 129c116..c6b7f2a 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -103,6 +103,57 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	return 0;
 }
 
+struct sync_stop_machine_data {
+	atomic_t begin;
+	atomic_t end;
+	int	 (*fn)(void *);
+	void	 *data;
+};
+
+static int __kprobes __sync_stop_machine(void *data)
+{
+	struct sync_stop_machine_data *ssmd;
+	int ret;
+
+	ssmd = (struct sync_stop_machine_data *)data;
+
+	/* Wait until all CPUs are ready before calling the function */
+	atomic_dec(&ssmd->begin);
+	smp_mb__after_atomic_dec();
+	while (atomic_read(&ssmd->begin)) {}
+
+	ret = ssmd->fn(ssmd->data);
+
+	/* Wait for all CPUs to finish 'fn' before returning */
+	smp_mb__before_atomic_dec();
+	atomic_dec(&ssmd->end);
+	while (atomic_read(&ssmd->end)) {}
+
+	return ret;
+}
+
+static int __kprobes sync_stop_machine(int (*fn)(void *), void *data)
+{
+	struct sync_stop_machine_data ssmd = {
+		.fn	= fn,
+		.data	= data
+	};
+	int num_cpus;
+	int ret;
+
+	get_online_cpus(); /* So number of CPUs can't change */
+
+	num_cpus = num_online_cpus();
+	atomic_set(&ssmd.begin, num_cpus);
+	atomic_set(&ssmd.end, num_cpus);
+	smp_wmb();
+
+	ret = stop_machine(__sync_stop_machine, &ssmd, &cpu_online_map);
+
+	put_online_cpus();
+	return ret;
+}
+
 #ifdef CONFIG_THUMB2_KERNEL
 
 /*
@@ -127,7 +178,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);
+		sync_stop_machine(set_t32_breakpoint, (void *)addr);
 	} else {
 		/* Word aligned 32-bit instruction can be written atomically */
 		u32 bkp = KPROBE_THUMB32_BREAKPOINT_INSTRUCTION;
@@ -190,7 +241,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);
+	sync_stop_machine(__arch_disarm_kprobe, p);
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
-- 
1.7.2.5

[PATCH] [RFC PATCH] ARM: kprobes: Make breakpoint setting/clearing SMP safe

[Rabin Vincent]

On Tue, Jul 12, 2011 at 18:10, Tixy <tixy@yxit.co.uk> wrote:
> Both these issues are known and the kprobe implementation uses
> stop_machine() to avoid them, however, this is not sufficient.
> stop_machine() does not perform any kind on synchronisation between CPUs
> so it it still possible for one CPU to call the breakpoint changing
> function before another CPU has been interrupted to do likewise.
>
> To fix this problem, this patch creates a new function
> sync_stop_machine() which ensures that all online CPUs execute the
> specified function at the same time.

AFAICS stop_machine() already does what you want.  When you use
stop_machine(), the actual call to your function is done (on each
CPU's stopper thread) using the stop_machine_cpu_stop() function,
which already has the synchronization between CPUs which you're
trying to do here.

[PATCH] [RFC PATCH] ARM: kprobes: Make breakpoint setting/clearing SMP safe

[Tixy]

On Wed, 2011-10-12 at 22:03 +0530, Rabin Vincent wrote:
> On Tue, Jul 12, 2011 at 18:10, Tixy <tixy@yxit.co.uk> wrote:
> > Both these issues are known and the kprobe implementation uses
> > stop_machine() to avoid them, however, this is not sufficient.
> > stop_machine() does not perform any kind on synchronisation between CPUs
> > so it it still possible for one CPU to call the breakpoint changing
> > function before another CPU has been interrupted to do likewise.
> >
> > To fix this problem, this patch creates a new function
> > sync_stop_machine() which ensures that all online CPUs execute the
> > specified function at the same time.
> 
> AFAICS stop_machine() already does what you want.  When you use
> stop_machine(), the actual call to your function is done (on each
> CPU's stopper thread) using the stop_machine_cpu_stop() function,
> which already has the synchronization between CPUs which you're
> trying to do here.

The only serialisation I can see is the wait_for_completion() in
__stop_cpus() which waits for the all CPUs to have executed the
requested function. I don't see anything which prevents one CPU from
starting (and finishing) the requested function before other CPU's are
ready for it. If this synchronisation existed it would have to be in
cpu_stopper_thread() as it pulls tasks off the work list.

In queue_stop_cpus_work() we have

	for_each_cpu(cpu, cpumask)
		cpu_stop_queue_work(&per_cpu(cpu_stopper, cpu),
				    &per_cpu(stop_cpus_work, cpu));

If the CPU executing this gets delayed by an ISR on each iteration of
the for_each_cpu loop then the work it queues for one CPU could have
completed before it gets around to queuing it for the next CPU.

-- 
Tixy

[PATCH] [RFC PATCH] ARM: kprobes: Make breakpoint setting/clearing SMP safe

[Tixy]

On Thu, 2011-10-13 at 08:40 +0100, Tixy wrote:
> On Wed, 2011-10-12 at 22:03 +0530, Rabin Vincent wrote:
> > On Tue, Jul 12, 2011 at 18:10, Tixy <tixy@yxit.co.uk> wrote:
> > > Both these issues are known and the kprobe implementation uses
> > > stop_machine() to avoid them, however, this is not sufficient.
> > > stop_machine() does not perform any kind on synchronisation between CPUs
> > > so it it still possible for one CPU to call the breakpoint changing
> > > function before another CPU has been interrupted to do likewise.
> > >
> > > To fix this problem, this patch creates a new function
> > > sync_stop_machine() which ensures that all online CPUs execute the
> > > specified function at the same time.
> > 
> > AFAICS stop_machine() already does what you want.  When you use
> > stop_machine(), the actual call to your function is done (on each
> > CPU's stopper thread) using the stop_machine_cpu_stop() function,
> > which already has the synchronization between CPUs which you're
> > trying to do here.
> 
> The only serialisation I can see is the wait_for_completion() in
> __stop_cpus() which waits for the all CPUs to have executed the
> requested function. I don't see anything which prevents one CPU from
> starting (and finishing) the requested function before other CPU's are
> ready for it. If this synchronisation existed it would have to be in
> cpu_stopper_thread() as it pulls tasks off the work list.
> 
> In queue_stop_cpus_work() we have
> 
> 	for_each_cpu(cpu, cpumask)
> 		cpu_stop_queue_work(&per_cpu(cpu_stopper, cpu),
> 				    &per_cpu(stop_cpus_work, cpu));
> 
> If the CPU executing this gets delayed by an ISR on each iteration of
> the for_each_cpu loop then the work it queues for one CPU could have
> completed before it gets around to queuing it for the next CPU.
> 

What I said above is wrong because I didn't see the whole picture.
queue_stop_cpus_work() isn't queueing the work we specify, it is used to
queue stop_machine_cpu_stop() on all running CPUs. This function then
synchronises all cores before executing our function with IRQs off.

Therefore, as Rabin said, the problem with kprobes I thought existed,
doesn't.

-- 
Tixy