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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>