linux-kernel - probe_event_disable()->synchronize_sched() (Was: tracing/uprobes: Revert "Support mix of ftrace and perf")

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140701193147.GA32492@redhat.com>
Date:	Tue, 1 Jul 2014 21:31:47 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Namhyung Kim <namhyung@...il.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Tom Zanussi <tom.zanussi@...ux.intel.com>,
	"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
	linux-kernel@...r.kernel.org
Subject: probe_event_disable()->synchronize_sched() (Was: tracing/uprobes:
	Revert "Support mix of ftrace and perf")

Namhyung, Masami,

Please look at the question below. Perhaps we discussed this before,
but I can recall nothing.

On 06/30, Oleg Nesterov wrote:
>
> Actually, I'll probably try to make the patch tomorrow. It looks simple
> enough, the main complication is CONFIG_PERF. And, to keep this patch
> simple, I won't try to optimize (say) the TP_FLAG_TRACE-comes-first
> case which could avoid uprobe_apply().

I regret very much I said this ;) OK, I'll probably try anyway, but...

> Yes, I still think it would be better to change the register/unregister
> API first, but I do not know when I do this ;)

OK, we can do this later.

But it turns out that trace_uprobe.c needs other cleanups, and I simply
can't uglify this code more without these cleanups... Starting from
set/clear TP_FLAG_*. I'll recheck, but it seems that there is no reason
to carefully set (say) TP_FLAG_TRACE before uprobe_register() and then
clear it if _register() fails. And uprobe_dispatcher() is very ugly if
is_ret_probe(). And more. So it needs a series.

-------------------------------------------------------------------------
And. I am puzzled by probe_event_disable()->synchronize_sched(). Why
do we need it? I mean, why we can't use call_rcu() ? The comment says
"synchronize with u{,ret}probe_trace_func" but it doesn't explain _why_
do we need to sync.

I thought that perhaps the caller needs to synch with the callbacks.
Say, __trace_remove_event_call() can destroy the data which can be used
by the callbacks. But no, this is only possible if we are going to call
uprobe_unregister(), and this adds the necessary serialization.

So why? Looks like, the only reason is instance_rmdir() which can do
TRACE_REG_UNREGISTER and after that destroy this ftrace_event_file?
But event_trace_del_tracer() also has synchronize_sched() right after
__ftrace_set_clr_event_nolock() ?

So please tell me why do we need this synchronize_sched ;) And imo
this should be documented.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/