linux-kernel - Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

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: <1177120325.1928.1394653861180.JavaMail.zimbra@efficios.com>
Date:	Wed, 12 Mar 2014 19:51:01 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	"Frank Ch. Eigler" <fche@...hat.com>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Berg <johannes.berg@...el.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	lttng-dev <lttng-dev@...ts.lttng.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
 set via debugfs

----- Original Message -----
> From: "Steven Rostedt" <rostedt@...dmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
> Cc: "Frank Ch. Eigler" <fche@...hat.com>, linux-kernel@...r.kernel.org, "Ingo Molnar" <mingo@...nel.org>, "Frederic
> Weisbecker" <fweisbec@...il.com>, "Andrew Morton" <akpm@...ux-foundation.org>, "Johannes Berg"
> <johannes.berg@...el.com>, "Linus Torvalds" <torvalds@...ux-foundation.org>, "Peter Zijlstra"
> <peterz@...radead.org>, "Thomas Gleixner" <tglx@...utronix.de>, "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
> "lttng-dev" <lttng-dev@...ts.lttng.org>, "Rusty Russell" <rusty@...tcorp.com.au>
> Sent: Wednesday, March 12, 2014 2:58:02 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 18:47:15 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
>  
> > > On module unload, the events are destroyed.
> > 
> > Isn't trace_event.c responsible for dealing with tracepoint probes rather
> > than call sites ? This is quite different. A tracepoint probe "foo" is only
> > located within a single module (the one you are unloading here). However,
> > if you try to unload a module that contains the callsite "foo", you have no
> > guarantee that no other modules also contain this callsite, and therefore
> > you cannot destroy the associated name, probe, nor data objects.
> 
> No module should have the same tracepoint name as another module.
> That's just broken. Although we do not technically enforce this, in
> practice that has been the case.

I agree with you on this.

I now notice that all the tracepoints within kernel headers have been removed,
this is why we're talking past each other. As soon as there is a tracepoint
within a static inline function in a header shared between modules, you end up
having instances of tracepoint call sites with the same names within different
modules.

So I understand that you wish to banish tracepoints from static inline
functions within headers to ensure they only appear within a single module.
This seems to be a step backward, but let's assume we stick to that rule.
Then how do you envision dealing with Link-Time Optimisations (LTO) ?

> > 
> > > 
> > > Thus, what your module should do, is exactly what event_remove() does.
> > > On module unload, you unregister any of the tracepoints that were
> > > registered. Just like any other module resource. If you request a
> > > resource on the behalf of a module, it is up to you to free it when the
> > > module is unloaded.
> > 
> > You seem to try to apply a logic that works in the case of the probes
> > defined by trace event to tracepoint call sites, but the fact is that
> > they are very different. Or again perhaps I'm just on the wrong track.
> > 
> > > 
> > > The tracepoint code will just destroy what it set up when the module
> > > was loaded. It's up to your module to clean up the allocations that you
> > > made when the module was loaded on unload. Just like we do for all
> > > other resources.
> > > 
> > > Mathieu, stop thinking that tracepoints are special. They are not.
> > 
> > I'm trying to understand how module going of tracepoint probes and
> > call sites can be considered the same. What am I missing ?
> 
> What creates the tracepoint probe? For all purposes, it should be
> either created on boot up (on core tracepoints), or when a module is
> loaded.

Yep, agreed.

> 
> Two modules should not have the same name. Is there any duplicate
> tracepoints you are aware of. Namespace collisions in tracepoints
> should be avoided, as that would cause people to trace things they did
> not intend on tracing.
> 
> That should be a new patch as well. Enforce unique tracepoint names.

There are no duplicate tracepoint that I'm aware of in the kernel (or
very few, and they are in similar functions in the same module).

This only leaves tracepoints in header files and the impact of LTO as
requirements for having tracepoint callsites with the same name across
modules.

Thoughts ?

Thanks,

Mathieu

> 
> -- Steve
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
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/