LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	bristot@redhat.com, "Naveen N . Rao" <naveen.n.rao@linux.ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
	David Miller <davem@davemloft.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [BUGFIX PATCH] kprobes: Fix to cancel optimizing/unoptimizing kprobes correctly
Date: Wed, 8 Jan 2020 12:02:49 +0900
Message-ID: <20200108120249.99d5a6d5201d1401e24447bc@kernel.org> (raw)
In-Reply-To: <20200107183907.3c87500a@gandalf.local.home>

On Tue, 7 Jan 2020 18:39:07 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue,  7 Jan 2020 23:42:24 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > optimize_kprobe() and unoptimize_kprobe() cancels if given kprobe
> > is on the optimizing_list or unoptimizing_list. However, since
> > commit f66c0447cca1 ("kprobes: Set unoptimized flag after
> > unoptimizing code") modified the update timing of the
> > KPROBE_FLAG_OPTIMIZED, it doesn't work as expected anymore.
> > 
> > The optimized_kprobe could be following states.
> > 
> > - [optimizing]: Before inserting jump instruction
> >   op.kp->flags has KPROBE_FLAG_OPTIMIZED and
> >   op->list is not empty.
> > 
> > - [optimized]: jump inserted
> >   op.kp->flags has KPROBE_FLAG_OPTIMIZED and
> >   op->list is empty.
> > 
> > - [unoptimizing]: Before removing jump instruction (including unused
> >   optprobe)
> >   op.kp->flags has KPROBE_FLAG_OPTIMIZED and
> >   op->list is not empty.
> > 
> > - [unoptimized]: jump removed
> >   op.kp->flags doesn't have KPROBE_FLAG_OPTIMIZED and
> >   op->list is empty.
> > 
> > Current code mis-expects [unoptimizing] state doesn't have
> > KPROBE_FLAG_OPTIMIZED, and that can cause wrong results.
> > 
> > This introduces optprobe_queued_unopt() to distinguish [optimizing]
> > and [unoptimizing] states and fixes logics in optimize_kprobe() and
> > unoptimize_kprobe().
> > 
> > Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Looks good.
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thank you!

> 
> 
> >  		return;
> >  	}
> > +
> >  	/* Optimized kprobe case */
> > -	if (force)
> > +	if (force) {
> >  		/* Forcibly update the code: this is a special case */
> >  		force_unoptimize_kprobe(op);
> > -	else {
> > +	} else {
> >  		list_add(&op->list, &unoptimizing_list);
> >  		kick_kprobe_optimizer();
> >  	}
> 
> I see you added some clean up to this patch.

Yeah, I felt somewhat uncomfortable for that. 

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-07 14:42 Masami Hiramatsu
2020-01-07 23:39 ` Steven Rostedt
2020-01-08  3:02   ` Masami Hiramatsu [this message]
2020-01-10  6:03 ` [tip: core/kprobes] kprobes: Fix optimize_kprobe()/unoptimize_kprobe() cancellation logic tip-bot2 for Masami Hiramatsu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200108120249.99d5a6d5201d1401e24447bc@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=anil.s.keshavamurthy@intel.com \
    --cc=ast@kernel.org \
    --cc=bp@alien8.de \
    --cc=bristot@redhat.com \
    --cc=davem@davemloft.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=naveen.n.rao@linux.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git