From: Oleg Nesterov <oleg@redhat.com> To: Steven Rostedt <rostedt@goodmis.org> Cc: LKML <linux-kernel@vger.kernel.org>, "zhangwei(Jovi)" <jovi.zhangwei@huawei.com>, Masami Hiramatsu <mhiramat@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, stable <stable@vger.kernel.org> Subject: Re: [PATCH] uprobes: Use synchronize_rcu() not synchronize_sched() Date: Fri, 10 Aug 2018 15:36:08 +0200 Message-ID: <20180810133608.GC3677@redhat.com> (raw) In-Reply-To: <20180810084832.70b9a62a@gandalf.local.home> On 08/10, Steven Rostedt wrote: > > On Fri, 10 Aug 2018 13:35:49 +0200 > Oleg Nesterov <oleg@redhat.com> wrote: > > > On 08/09, Steven Rostedt wrote: > > > > > > --- a/kernel/trace/trace_uprobe.c > > > +++ b/kernel/trace/trace_uprobe.c > > > @@ -952,7 +952,7 @@ probe_event_disable(struct trace_uprobe *tu, struct trace_event_file *file) > > > > > > list_del_rcu(&link->list); > > > /* synchronize with u{,ret}probe_trace_func */ > > > - synchronize_sched(); > > > + synchronize_rcu(); > > > > Can't we change uprobe_trace_func() and uretprobe_trace_func() to use > > rcu_read_lock_sched() instead? It is more cheap. > > Is it? rcu_read_lock_sched() is a preempt_disable(), which is just raw_cpu_inc() > where > rcu_read_lock() may just be a task counter increment. and __rcu_read_unlock() is more heavy. OK, I agree, this doesn't really matter. > > Hmm. probe_event_enable() does list_del + kfree on failure, this doesn't > > look right... Not only because kfree() can race with list_for_each_entry_rcu(), > > we should not put the 1st link on list until uprobe_buffer_enable(). > > > > Does the patch below make sense or I am confused? > > I guess the question is, if it isn't enabled, are there any users or > even past users still running. Note that uprobe_register() is not "atomic". To simplify, suppose we have 2 tasks T1 and T2 running the probed binary. So we are going to do install_breakpoint(T1->mm) + install_breakpoint(T2->mm). If the 2nd install_breakpoint() fails for any reason, _register() will do remove_breakpoint(T1->mm) and return the error. However, T1 can hit this bp right after install_breakpoint(T1->mm), so it can call uprobe_trace_func() before list_del(&link->list). OK, even if I am right this is mostly theoretical. Oleg.
next prev parent reply index Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-09 20:05 Steven Rostedt 2018-08-10 11:35 ` Oleg Nesterov 2018-08-10 11:42 ` Oleg Nesterov 2018-08-10 12:48 ` Steven Rostedt 2018-08-10 13:36 ` Oleg Nesterov [this message] 2018-08-10 13:44 ` Steven Rostedt 2018-08-10 14:06 ` Oleg Nesterov
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=20180810133608.GC3677@redhat.com \ --to=oleg@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=jovi.zhangwei@huawei.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mhiramat@kernel.org \ --cc=namhyung@kernel.org \ --cc=rostedt@goodmis.org \ --cc=stable@vger.kernel.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 git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.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