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: <1666649564.1944.1394654290265.JavaMail.zimbra@efficios.com>
Date:	Wed, 12 Mar 2014 19:58:10 +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 3:30:06 PM
> Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs
> 
> On Wed, 12 Mar 2014 14:58:02 -0400
> Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> > 
> > 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.
> 
> This may be why you are not understanding what I want. It's the way
> things are implemented today, which I believe are wrong. I see what you
> did. You have probes that are registered, and tracepoints that are
> where the code lies. You just add and remove probes from a hash list,
> and then you loop through all the tracepoints seeing if the iter->name
> matches a probe->name.
> 
> I'm fine with keeping the probe separate, but there really should be
> no more than just a one to one mapping between probes and tracepoints.
> Have the probe point to the matching tracepoint. The probe is
> registered, it enables the tracepoint static key, when it's ref count
> goes to zero, it disables the tracepoint static key. We can get rid of
> that loop then, as well as the duplicate names between probes and
> tracepoints.

Right there, this is not possible for a few reasons, namely:

- loop unrolling performed by the compiler can duplicate a tracepoint,
  even if it is only there once in the source code,
- inlining performed by the compiler may do the same,
- LTO, whenever it will start being used for the kernel, may do the same,
  and also spread call sites across modules.

There can be no 1 to 1 mapping between a probe function and a callsite
due to those compilers optimisations, even if we enforce the strictest
coding style rules possible on their use.

Thanks,

Mathieu


> 
> Here's the steps we should take:
> 
> 1) Prevent duplicate tracepoints. They are just namespace collisions
> that we already try to avoid. How to do this? We may need to add a
> hlist_node to the tracepoint structure, and keep them in a hash by name.
> Check for collisions when the name is added to the hash.
> 
> 2) Change the way tracepoints are enabled. Do not do a loop of all
> tracepoints, but instead have the first probe of a tracepoint enable
> it, and the last one to disable it. This would require a pointer from
> the probe to the tracepoint it represents. Again, it should not
> represent more than one.
> 
> 3) On module unload, it would be the responsibility of the user to
> unload all the tracepoints that may have been enabled for a module. We
> can add a mod pointer in the probe to make this easier, as well as to
> the tp_module structure.
> 
> The way tracepoints are today are to handle two completely different
> tracepoints with the same name. That should be avoided, and will make
> things much less complex.
> 
> Then you can easily handle the accounting of modules loading and
> unloading in your module, and the tracepoint code will match what the
> rest of the kernel does for resource management.
> 
> -- 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/