Re: [PATCH -tip v3 7/7] kprobes: Use synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT

Masami Hiramatsu
2017-09-29T07:29:42Z

On Thu, 28 Sep 2017 09:22:20 +0200
Ingo Molnar <mingo@kernel.org> wrote:

>
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > To enable jump optimized probe with CONFIG_PREEMPT, use
> > synchronize_rcu_tasks() to wait for all tasks preempted
> > on trampoline code back on track.
>
> This sentence does not parse. It's missing a verb, but I'm not sure.

Hmm, how about this?

Use synchthnize_rcu_tasks() to wait for all tasks preempted
on trampoline code back on track so that jump optimized probe
can be enabled with CONFIG_PREEMPT.

>
> > Since the jump optimized kprobes can replace multiple
> > instructions, there can be tasks which are preempted
> > on the 2nd (or 3rd) instructions. If the kprobe
> > replaces those instructions by a jump instruction,
> > when those tasks back to the preempted place, it is
> > a middle of the jump instruction and causes a kernel
> > panic.
>
>
> Again, sentence appears to be missing a verb and also an adjective I think.
>

Hmm, I couldn't understand, I think you are pointing below
sentence,
----
If the kprobe replaces those instructions by a jump instruction,
when those tasks back to the preempted place, it is a middle of
the jump instruction and causes a kernel panic.
----

Of course "If" and "when" look ugly, but both have verb...

> > To avoid such tragedies in advance, kprobe optimizer
> > prepare a detour route using normal kprobe (e.g.
> > int3 breakpoint on x86), and wait for the tasks which
> > is interrrupted on such place by synchronize_sched()
> > when CONFIG_PREEMPT=n.
>
> s/tragedies/mishaps

I got it.

>
> Part after the first comma does not parse.

Yeah, some typos, but

kprobe optimizer prepares a detour route using normal kprobe ()
and waits for the tasks, which is interrupted on such place, by
synchronize_sched(), when CONFIG_PREEMPT=n.

will be able to parsed. ( at least google translate can ...)

>
> Also the way to refer to kprobes is "kprobes" and "normal kprobes".
> Use 'kprobe' only when talking about a specific kprobe instance or such.
> You use this correctly later on in the changelog ...
>
> > If CONFIG_PREEMPT=y, things be more complicated, because
>
> s/be/are or s/be/get

thanks, get is preferred :)

>
> > such interrupted thread can be preempted (other thread
> > can be scheduled in interrupt handler.) So, kprobes
>
> full stop in the wrong place.
>
> > optimizer has to wait for those tasks scheduled normally.
>
> missing verb.

kprobe optimizer must wait for those ...

will it work?

>
> > In this case we can use synchronize_rcu_tasks() which
> > ensures that all preempted tasks back on track and
> > schedule it.
>
> More careful changelogs please.
>
> > + * are done. Because optprobe may modify multiple instructions,
> > + * there is a chance that the Nth instruction is interrupted. In that
> > + * case, running interrupt can return to the Nth byte of jump
> > + * instruction. This can be avoided by waiting for returning of
> > + * such interrupts, since (until here) the first byte of the optimized
> > + * probe is already replaced with normal kprobe (sw breakpoint) and
> > + * all threads which reach to the probed address will hit it and
> > + * bypass the copied instructions (instead of executing the original.)
> > + * With CONFIG_PREEMPT, such interrupts can be preepmted. To wait
> > + * for such thread, we will use synchronize_rcu_tasks() which ensures
> > + * all preeempted tasks are scheduled normally (not preempted).
> > + * So we can ensure there is no threads preempted at probed address.
>
> What? Interrupts cannot be preempted.

Steve, could you correct me if I'm wrong. I thought if the kernel is
compiled with CONFIG_PREEMPT=y, even in the kernel, it can be preempted
suddenly. It means timer interrupt occurs at kernel path and it yield
to new task (=preempt.) Do I miss something?

>
> Also, "To wait for such threads", or "To wait for such a thread".

OK,

Thank you,

>
> Thanks,
>
> Ingo

--
Masami Hiramatsu <mhiramat@kernel.org>