DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9561921869
Date: Fri, 29 Sep 2017 00:29:38 -0700
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: mingo@redhat.com, x86@kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
        Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
        Alexei Starovoitov <ast@kernel.org>, Alexei Starovoitov <ast@fb.com>
Subject: Re: [PATCH -tip v3 7/7] kprobes: Use synchronize_rcu_tasks() for
 optprobe with CONFIG_PREEMPT
Message-Id: <20170929002938.64cb852d6d921ec013d7f052@kernel.org>
In-Reply-To: <20170928072220.iv5subwl4u4zcqcp@gmail.com>
References: <150581509713.32348.1905525476438163954.stgit@devbox>
        <150581537997.32348.14125457458719053753.stgit@devbox>
        <20170928072220.iv5subwl4u4zcqcp@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4023
Lines: 130

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>