linux-kernel - Re: probe_event_disable()->synchronize

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: <87pphn45y6.fsf@sejong.aot.lge.com>
Date:	Thu, 03 Jul 2014 09:54:57 +0900
From:	Namhyung Kim <namhyung@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	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: Re: probe_event_disable()->synchronize_sched()

Hi Oleg,

On Tue, 1 Jul 2014 21:31:47 +0200, Oleg Nesterov wrote:
> Namhyung, Masami,
>
> Please look at the question below. Perhaps we discussed this before,
> but I can recall nothing.

I'm not sure I grok the code enough to answer your question, but...

>
>
> 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.

Okay, I'd like to see it. :)


>
> -------------------------------------------------------------------------
> 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.

It looks like the code was copied from trace_kprobe.c file.  But IIUC,
unlike kprobes, uprobe events are always called in a process context.

Also u{,ret}probe_trace_func() call handlers under rcu_read_lock() not
rcu_read_lock_sched() so I guess the synchronize_sched() can go.

Thanks,
Namhyung


>
> 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/