Questions about kprobes implementation on SMP and preemptible kernels

[Quentin Barnes]

>Hi Quentin,
>
>>I'm wondering if preempt_disable() is also superfluous in
>>kprobe_handler() too for architectures where interrupts are disabled
>>throughout the exception and into resuming from the exception when
>>the previous context had been modified to resume with interrupts
>>disabled.  Now this model may not be used on all architectures,
>>but on the ones that it is, is this true that since interrupts are
>>disabled that preempt_disable() is superfluous in kprobe_handler()?
>
>Disabling interrupts is just one way to make sure that our process
>doesn't get preempted. The other ways of getting preempted are
>mentioned towards the end in:
>http://www.samspublishing.com/articles/article.asp?p=101760&seqNum=3&rl=1

That's a nice summary.

>Since interrupts are disabled in the undef exception context, we can
>be pretty sure that we won't get scheduled out (and thus preempted).
>This however doesn't mean that we cannot encounter code (in the user
>pre-handler, for example) which may mark the current process for
>rescheduling. If that happens, then the current process will be
>preempted at the next preemption checkpoint. If that check point
>happens to fall in the kprobe code path with preemption and interrupts
>enabled, then we're in trouble.

Most definitely.

However, interrupts can only be safely reenabled in the kprobe_handler()
path under some very strict guidelines which will not always be true.

>I'm not sure if this is totally
>relevant to your larger point of questioning the presence of
>preempt_disable() in kprobe_handler with interrupts disabled, but it
>adds as a check in case you plan to run things in kprobe_handler with
>interrupts enabled (and possibly skipping over preempt_disable).

My question was just to ensure that if interrupts were disabled that
there was no rational reason to acquire the preemption lock too.

>>Q #3:
>>The ARM kprobes model uses an undefined instruction for its
>>kprobe.  This is necessary since ARM's breakpoint instruction (BKPT)
>>triggers entry into the same CPU service mode as the kernel runs
>>in.  On ARM, this is bad to do.  No problem though since undefined
>>instructions have their own mode.  Due to this, kprobe_handler() is
>>called from ARM's undefined instruction handler, do_undefinstr().
>>Currently interrupts remain disabled all the way through the
>>kprobe's undef instruction exception handling into kprobe_handler()
>>and up through returning.  I would like to eventually change the
>>code so that interrupts can be re-enabled at some point to reduce
>>interrupt latency.
>
>If you're planning to run some part of the kprobe code-path with
>interrupts enabled, then that code must be sandwitched between
>preempt_disable/enable.

Yes.

If the code that triggered the kprobe'd instruction had interrupts
disabled, we can never re-enable them in the kprobe handler, else
we broken that critical section.  They have to remain disabled
throughout the entire handler.

If the kprobe'd context we came from had interrupts enabled, we have
the opportunity to re-enable them in kprobe_handler() call path.

>E.g, if you plan to single-step with interrupts enabled:
>Consider the following user handler for a jprobe (jp->entry = foo):
>
>1. int foo(/* args */) {
>2. 	woo(); /* marks current process's NEED_RESCHED bit */
>3. 	spin_lock(lock); /* preempt_count = 1*/
>4.	/* critical section */
>5.	spin_unlock(lock); /* preempt_count = 0; rechedule! */
>6.	jprobe_return();
>7. }
>
>Assuming that such a foo() can exists (hope I haven't got it
>wrong!)...(5) would lead to rescheduling and the current process might
>end up on another processor's run-queue. In that case when foo returns
>to kprobe context in (6), it'll end up using an unrelated per-cpu
>kprobe control block. A simple preempt_disable() in kprobe_handler
>would prevent all this. Just wanted to add this scenario to your
>check-list :D

Oh, yes.  The point would be to trade a higher priority lock
(holding interrupts disabled) for a lower priority lock (holding
scheduling disabled).  Throughout kprobes exception handling, you
have to hold one lock or the other at all times.  And if the context
you came from already had the elevated priority of interrupts
disabled, you can't drop your priority in the handler by forcing
interrupts re-enabled without having bad things happen.

>Regards
>Abhishek Sagar

Quentin