[tracepoint] cargo-culting considered harmful...

[tracepoint] cargo-culting considered harmful...

[Al Viro]

In samples/tracepoints/tracepoint-probe-sample.c:
/*
 * Here the caller only guarantees locking for struct file and struct inode.
 * Locking must therefore be done in the probe to use the dentry.
 */
static void probe_subsys_event(void *ignore,   
                               struct inode *inode, struct file *file)
{
        path_get(&file->f_path);
        dget(file->f_path.dentry);
        printk(KERN_INFO "Event is encountered with filename %s\n",
                file->f_path.dentry->d_name.name);
        dput(file->f_path.dentry);
        path_put(&file->f_path);
}

note that
	* file->f_path is already pinned down by open(), path_get() does not
provide anything extra.
	* file->f_path.dentry is already pinned by open() *and* path_get()
just above that dget().
	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
whether it's done once or thrice.

I do realize that it's just an example, but perhaps we should rename that
file to match the contents?  The only question is whether it should be
git mv samples/tracepoints/{tracepoint-probe-sample,cargo-cult}.c
or git mv samples cargo-cult...

					Al, seriously peeved.

Re: [tracepoint] cargo-culting considered harmful...

[Steven Rostedt]

On Wed, Jan 23, 2013 at 10:55:24PM +0000, Al Viro wrote:
> In samples/tracepoints/tracepoint-probe-sample.c:
> /*
>  * Here the caller only guarantees locking for struct file and struct inode.
>  * Locking must therefore be done in the probe to use the dentry.
>  */
> static void probe_subsys_event(void *ignore,   
>                                struct inode *inode, struct file *file)
> {
>         path_get(&file->f_path);
>         dget(file->f_path.dentry);
>         printk(KERN_INFO "Event is encountered with filename %s\n",
>                 file->f_path.dentry->d_name.name);
>         dput(file->f_path.dentry);
>         path_put(&file->f_path);
> }
> 
> note that
> 	* file->f_path is already pinned down by open(), path_get() does not
> provide anything extra.
> 	* file->f_path.dentry is already pinned by open() *and* path_get()
> just above that dget().
> 	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
> whether it's done once or thrice.
> 
> I do realize that it's just an example, but perhaps we should rename that
> file to match the contents?  The only question is whether it should be
> git mv samples/tracepoints/{tracepoint-probe-sample,cargo-cult}.c
> or git mv samples cargo-cult...

I wonder if we should just remove the samples/tracepoints/ all together.
The tracepoint code is now only used internally by the trace_event code,
and there should not be any users of tracepoints directly.

-- Steve

Re: [tracepoint] cargo-culting considered harmful...

[Andrew Morton]

On Wed, 23 Jan 2013 22:55:24 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> In samples/tracepoints/tracepoint-probe-sample.c:
> /*
>  * Here the caller only guarantees locking for struct file and struct inode.
>  * Locking must therefore be done in the probe to use the dentry.
>  */
> static void probe_subsys_event(void *ignore,   
>                                struct inode *inode, struct file *file)
> {
>         path_get(&file->f_path);
>         dget(file->f_path.dentry);
>         printk(KERN_INFO "Event is encountered with filename %s\n",
>                 file->f_path.dentry->d_name.name);
>         dput(file->f_path.dentry);
>         path_put(&file->f_path);
> }
> 
> note that
> 	* file->f_path is already pinned down by open(), path_get() does not
> provide anything extra.
> 	* file->f_path.dentry is already pinned by open() *and* path_get()
> just above that dget().
> 	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
> whether it's done once or thrice.

I guess the first two are obvious (or at least, expected).  But the
third isn't.

Where should a kernel developer go to learn these things? 
include/linux/dcache.h doesn't mention d_name locking rules, nor does
Documentation/filesystems/vfs.txt.

Re: [tracepoint] cargo-culting considered harmful...

[Al Viro]

On Wed, Jan 23, 2013 at 03:51:47PM -0800, Andrew Morton wrote:

> > note that
> > 	* file->f_path is already pinned down by open(), path_get() does not
> > provide anything extra.
> > 	* file->f_path.dentry is already pinned by open() *and* path_get()
> > just above that dget().
> > 	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
> > whether it's done once or thrice.
> 
> I guess the first two are obvious (or at least, expected).  But the
> third isn't.

->d_name.name is changed by rename() (as one could expect).  Grabbing
a reference to dentry will not prevent rename() from happening.  ->i_mutex
on parent will, but you either need to play with retries (grab reference
to parent, grab ->i_mutex, check that it's still our parent, if we'd lost
the race and someone had renamed the sucker - unlock ->i_mutex, dput,
repeat) *or* to have our dentry looked up with parent locked, with ->i_mutex
on said parent still held (which happens to cover the majority of valid
uses in fs code - ->lookup(), ->create(), ->unlink(), rename(), etc. are
all called that way, so the name of dentry passed to such methods is stable
for the duration of the method).

->d_lock on dentry is also sufficient, but that obviously means that you
can't block while holding it.

> Where should a kernel developer go to learn these things? 
> include/linux/dcache.h doesn't mention d_name locking rules, nor does
> Documentation/filesystems/vfs.txt.

See directory locking rules in there; the crucial point is that dentry
name is changed by rename() *and* that results of a race can be worse than
just running into a partially rewritten name - long names are allocated
separately and walking through a stale pointer you might end up in freed
memory.

It's a mess, unfortunately, and $BIGNUM other uses of ->i_mutex make it only
nastier.  Once in a while I go hunting for races in that area, usally with
a bunch of fixes coming out of such run ;-/

Re: [tracepoint] cargo-culting considered harmful...

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Wed, Jan 23, 2013 at 10:55:24PM +0000, Al Viro wrote:
> > In samples/tracepoints/tracepoint-probe-sample.c:
> > /*
> >  * Here the caller only guarantees locking for struct file and struct inode.
> >  * Locking must therefore be done in the probe to use the dentry.
> >  */
> > static void probe_subsys_event(void *ignore,   
> >                                struct inode *inode, struct file *file)
> > {
> >         path_get(&file->f_path);
> >         dget(file->f_path.dentry);
> >         printk(KERN_INFO "Event is encountered with filename %s\n",
> >                 file->f_path.dentry->d_name.name);
> >         dput(file->f_path.dentry);
> >         path_put(&file->f_path);
> > }
> > 
> > note that
> > 	* file->f_path is already pinned down by open(), path_get() does not
> > provide anything extra.
> > 	* file->f_path.dentry is already pinned by open() *and* path_get()
> > just above that dget().
> > 	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
> > whether it's done once or thrice.
> > 
> > I do realize that it's just an example, but perhaps we should rename that
> > file to match the contents?  The only question is whether it should be
> > git mv samples/tracepoints/{tracepoint-probe-sample,cargo-cult}.c
> > or git mv samples cargo-cult...
> 
> I wonder if we should just remove the samples/tracepoints/ all together.
> The tracepoint code is now only used internally by the trace_event code,
> and there should not be any users of tracepoints directly.

Yep, I'd be OK with removing this example, since now all users are
expected to user TRACE_EVENT(), which is built on top of tracepoints.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [tracepoint] cargo-culting considered harmful...

[Mathieu Desnoyers]

* Al Viro (viro@ZenIV.linux.org.uk) wrote:
> On Wed, Jan 23, 2013 at 03:51:47PM -0800, Andrew Morton wrote:
> 
> > > note that
> > > 	* file->f_path is already pinned down by open(), path_get() does not
> > > provide anything extra.
> > > 	* file->f_path.dentry is already pinned by open() *and* path_get()
> > > just above that dget().
> > > 	* ->d_name.name *IS* *NOT* *PROTECTED* by pinning dentry down,
> > > whether it's done once or thrice.
> > 
> > I guess the first two are obvious (or at least, expected).  But the
> > third isn't.

Hi Al,

I agree that the tracepoint example should be removed. There is one
extra piece of module code I think would require fixing (see below).

> 
> ->d_name.name is changed by rename() (as one could expect).  Grabbing
> a reference to dentry will not prevent rename() from happening.  ->i_mutex
> on parent will, but you either need to play with retries (grab reference
> to parent, grab ->i_mutex, check that it's still our parent, if we'd lost
> the race and someone had renamed the sucker - unlock ->i_mutex, dput,
> repeat) *or* to have our dentry looked up with parent locked, with ->i_mutex
> on said parent still held (which happens to cover the majority of valid
> uses in fs code - ->lookup(), ->create(), ->unlink(), rename(), etc. are
> all called that way, so the name of dentry passed to such methods is stable
> for the duration of the method).
> 
> ->d_lock on dentry is also sufficient, but that obviously means that you
> can't block while holding it.
> 
> > Where should a kernel developer go to learn these things? 
> > include/linux/dcache.h doesn't mention d_name locking rules, nor does
> > Documentation/filesystems/vfs.txt.
> 
> See directory locking rules in there; the crucial point is that dentry
> name is changed by rename() *and* that results of a race can be worse than
> just running into a partially rewritten name - long names are allocated
> separately and walking through a stale pointer you might end up in freed
> memory.
> 
> It's a mess, unfortunately, and $BIGNUM other uses of ->i_mutex make it only
> nastier.  Once in a while I go hunting for races in that area, usally with
> a bunch of fixes coming out of such run ;-/

In the light of what you are saying here, am I right to think that the
following code is broken wrt locking wrt use of
filp->f_dentry->d_name.name ?

static
void lttng_enumerate_task_fd(struct lttng_session *session,
                struct task_struct *p, char *tmp)
{
        struct fdtable *fdt;
        struct file *filp;
        unsigned int i;
        const unsigned char *path;

        task_lock(p);
        if (!p->files)
                goto unlock_task;
        spin_lock(&p->files->file_lock);
        fdt = files_fdtable(p->files);
        for (i = 0; i < fdt->max_fds; i++) {
                filp = fcheck_files(p->files, i);
                if (!filp)
                        continue;
                path = d_path(&filp->f_path, tmp, PAGE_SIZE);
                /* Make sure we give at least some info */
                trace_lttng_statedump_file_descriptor(session, p, i,
                        IS_ERR(path) ?
                                filp->f_dentry->d_name.name :
                                path);
        }
        spin_unlock(&p->files->file_lock);
unlock_task:
        task_unlock(p);
}

Since tracepoints never block, holding the ->d_lock around the
trace_lttng_statedump_file_descriptor() tracepoint should probably be
enough to make it correct. Am I missing anything ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [tracepoint] cargo-culting considered harmful...

[Steven Rostedt]

On Fri, 2013-01-25 at 09:38 -0500, Mathieu Desnoyers wrote:

> Yep, I'd be OK with removing this example, since now all users are
> expected to user TRACE_EVENT(), which is built on top of tracepoints.

Can I  get your Acked-by for the following patch?

-- Steve

commit 867a31fab0a064e54147371425f9fdef933e3d1d
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Fri Jan 25 09:46:36 2013 -0500

    tracing: Remove tracepoint sample code
    
    The tracepoint sample code was used to teach developers how to
    create their own tracepoints. But now the trace_events have been
    added as a higher level that is used directly by developers today.
    
    Only the trace_event code should use the tracepoint interface
    directly and no new tracepoints should be added.
    
    Besides, the example had a race condition with the use of the
     ->d_name.name dentry field, as pointed out by Al Viro.
    
    Best just to remove the code so it wont be used by other developers.
    
    Cc: Al Viro <viro@ZenIV.linux.org.uk>
    Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/samples/Kconfig b/samples/Kconfig
index 7b6792a..6181c2c 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -5,12 +5,6 @@ menuconfig SAMPLES
 
 if SAMPLES
 
-config SAMPLE_TRACEPOINTS
-	tristate "Build tracepoints examples -- loadable modules only"
-	depends on TRACEPOINTS && m
-	help
-	  This build tracepoints example modules.
-
 config SAMPLE_TRACE_EVENTS
 	tristate "Build trace_events examples -- loadable modules only"
 	depends on EVENT_TRACING && m
diff --git a/samples/Makefile b/samples/Makefile
index 5ef08bb..1a60c62 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -1,4 +1,4 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/ \
+obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ trace_events/ \
 			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
diff --git a/samples/tracepoints/Makefile b/samples/tracepoints/Makefile
deleted file mode 100644
index 36479ad..0000000
--- a/samples/tracepoints/Makefile
+++ /dev/null
@@ -1,6 +0,0 @@
-# builds the tracepoint example kernel modules;
-# then to use one (as root):  insmod <module_name.ko>
-
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-sample.o
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample.o
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample2.o
diff --git a/samples/tracepoints/tp-samples-trace.h b/samples/tracepoints/tp-samples-trace.h
deleted file mode 100644
index 4d46be9..0000000
--- a/samples/tracepoints/tp-samples-trace.h
+++ /dev/null
@@ -1,11 +0,0 @@
-#ifndef _TP_SAMPLES_TRACE_H
-#define _TP_SAMPLES_TRACE_H
-
-#include <linux/proc_fs.h>	/* for struct inode and struct file */
-#include <linux/tracepoint.h>
-
-DECLARE_TRACE(subsys_event,
-	TP_PROTO(struct inode *inode, struct file *file),
-	TP_ARGS(inode, file));
-DECLARE_TRACE_NOARGS(subsys_eventb);
-#endif
diff --git a/samples/tracepoints/tracepoint-probe-sample.c b/samples/tracepoints/tracepoint-probe-sample.c
deleted file mode 100644
index 744c0b9..0000000
--- a/samples/tracepoints/tracepoint-probe-sample.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*
- * tracepoint-probe-sample.c
- *
- * sample tracepoint probes.
- */
-
-#include <linux/module.h>
-#include <linux/file.h>
-#include <linux/dcache.h>
-#include "tp-samples-trace.h"
-
-/*
- * Here the caller only guarantees locking for struct file and struct inode.
- * Locking must therefore be done in the probe to use the dentry.
- */
-static void probe_subsys_event(void *ignore,
-			       struct inode *inode, struct file *file)
-{
-	path_get(&file->f_path);
-	dget(file->f_path.dentry);
-	printk(KERN_INFO "Event is encountered with filename %s\n",
-		file->f_path.dentry->d_name.name);
-	dput(file->f_path.dentry);
-	path_put(&file->f_path);
-}
-
-static void probe_subsys_eventb(void *ignore)
-{
-	printk(KERN_INFO "Event B is encountered\n");
-}
-
-static int __init tp_sample_trace_init(void)
-{
-	int ret;
-
-	ret = register_trace_subsys_event(probe_subsys_event, NULL);
-	WARN_ON(ret);
-	ret = register_trace_subsys_eventb(probe_subsys_eventb, NULL);
-	WARN_ON(ret);
-
-	return 0;
-}
-
-module_init(tp_sample_trace_init);
-
-static void __exit tp_sample_trace_exit(void)
-{
-	unregister_trace_subsys_eventb(probe_subsys_eventb, NULL);
-	unregister_trace_subsys_event(probe_subsys_event, NULL);
-	tracepoint_synchronize_unregister();
-}
-
-module_exit(tp_sample_trace_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint Probes Samples");
diff --git a/samples/tracepoints/tracepoint-probe-sample2.c b/samples/tracepoints/tracepoint-probe-sample2.c
deleted file mode 100644
index 9fcf990..0000000
--- a/samples/tracepoints/tracepoint-probe-sample2.c
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * tracepoint-probe-sample2.c
- *
- * 2nd sample tracepoint probes.
- */
-
-#include <linux/module.h>
-#include <linux/fs.h>
-#include "tp-samples-trace.h"
-
-/*
- * Here the caller only guarantees locking for struct file and struct inode.
- * Locking must therefore be done in the probe to use the dentry.
- */
-static void probe_subsys_event(void *ignore,
-			       struct inode *inode, struct file *file)
-{
-	printk(KERN_INFO "Event is encountered with inode number %lu\n",
-		inode->i_ino);
-}
-
-static int __init tp_sample_trace_init(void)
-{
-	int ret;
-
-	ret = register_trace_subsys_event(probe_subsys_event, NULL);
-	WARN_ON(ret);
-
-	return 0;
-}
-
-module_init(tp_sample_trace_init);
-
-static void __exit tp_sample_trace_exit(void)
-{
-	unregister_trace_subsys_event(probe_subsys_event, NULL);
-	tracepoint_synchronize_unregister();
-}
-
-module_exit(tp_sample_trace_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint Probes Samples");
diff --git a/samples/tracepoints/tracepoint-sample.c b/samples/tracepoints/tracepoint-sample.c
deleted file mode 100644
index f4d89e0..0000000
--- a/samples/tracepoints/tracepoint-sample.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/* tracepoint-sample.c
- *
- * Executes a tracepoint when /proc/tracepoint-sample is opened.
- *
- * (C) Copyright 2007 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
- *
- * This file is released under the GPLv2.
- * See the file COPYING for more details.
- */
-
-#include <linux/module.h>
-#include <linux/sched.h>
-#include <linux/proc_fs.h>
-#include "tp-samples-trace.h"
-
-DEFINE_TRACE(subsys_event);
-DEFINE_TRACE(subsys_eventb);
-
-struct proc_dir_entry *pentry_sample;
-
-static int my_open(struct inode *inode, struct file *file)
-{
-	int i;
-
-	trace_subsys_event(inode, file);
-	for (i = 0; i < 10; i++)
-		trace_subsys_eventb();
-	return -EPERM;
-}
-
-static const struct file_operations mark_ops = {
-	.open = my_open,
-	.llseek = noop_llseek,
-};
-
-static int __init sample_init(void)
-{
-	printk(KERN_ALERT "sample init\n");
-	pentry_sample = proc_create("tracepoint-sample", 0444, NULL,
-		&mark_ops);
-	if (!pentry_sample)
-		return -EPERM;
-	return 0;
-}
-
-static void __exit sample_exit(void)
-{
-	printk(KERN_ALERT "sample exit\n");
-	remove_proc_entry("tracepoint-sample", NULL);
-}
-
-module_init(sample_init)
-module_exit(sample_exit)
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint sample");

Re: [tracepoint] cargo-culting considered harmful...

[Al Viro]

On Fri, Jan 25, 2013 at 09:49:53AM -0500, Mathieu Desnoyers wrote:
> static
> void lttng_enumerate_task_fd(struct lttng_session *session,
>                 struct task_struct *p, char *tmp)
> {
>         struct fdtable *fdt;
>         struct file *filp;
>         unsigned int i;
>         const unsigned char *path;
> 
>         task_lock(p);
>         if (!p->files)
>                 goto unlock_task;
>         spin_lock(&p->files->file_lock);
>         fdt = files_fdtable(p->files);
>         for (i = 0; i < fdt->max_fds; i++) {
>                 filp = fcheck_files(p->files, i);
>                 if (!filp)
>                         continue;
>                 path = d_path(&filp->f_path, tmp, PAGE_SIZE);
>                 /* Make sure we give at least some info */
>                 trace_lttng_statedump_file_descriptor(session, p, i,
>                         IS_ERR(path) ?
>                                 filp->f_dentry->d_name.name :
>                                 path);
>         }
>         spin_unlock(&p->files->file_lock);
> unlock_task:
>         task_unlock(p);
> }

*cringe*

a) yes, it needs d_lock for that ->d_name access
b) iterate_fd() is there for purpose; use it, instead of open-coding the
damn loop.  Something like

struct ctx {
	char *page;
	struct lttng_session *session,
	struct task_struct *p;
};
	
static int dump_one(void *p, struct file *file, unsigned fd)
{
	struct ctx *ctx = p;
	const char *s = d_path(&file->f_path, ctx->page, PAGE_SIZE);
	struct dentry *dentry;
	if (!IS_ERR(s)) {
		trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd, s);
		return 0;
	}
	/* Make sure we give at least some info */
	dentry = file->f_path.dentry;
	spin_lock(&dentry->d_lock);
	trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd,
		dentry->d_name);
	spin_unlock(&dentry->d_lock);
	return 0;
}

...
	task_lock(p);
	iterate_fd(p->files, 0, dump_one, &(struct ctx){tmp, session, p});
	task_unlock(p);

assuming it wouldn't be better to pass tmp/session/p as the single pointer
to struct in the first place - I don't know enough about the callers of
that sucker to tell.  And yes, iterate_fd() will DTRT if given NULL as the
first argument.  The second argument is "which descriptor should I start
from?", callback is called for everything present in the table starting from
that place until it returns non-zero or the end of table is reached...

PS: people really ought to be forced to read their code aloud over the phone -
that would rapidly improve the choice of identifiers ;-)

Re: [tracepoint] cargo-culting considered harmful...

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2013-01-25 at 09:38 -0500, Mathieu Desnoyers wrote:
> 
> > Yep, I'd be OK with removing this example, since now all users are
> > expected to user TRACE_EVENT(), which is built on top of tracepoints.
> 
> Can I  get your Acked-by for the following patch?

Sure,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

Mathieu

> 
> -- Steve
> 
> commit 867a31fab0a064e54147371425f9fdef933e3d1d
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Fri Jan 25 09:46:36 2013 -0500
> 
>     tracing: Remove tracepoint sample code
>     
>     The tracepoint sample code was used to teach developers how to
>     create their own tracepoints. But now the trace_events have been
>     added as a higher level that is used directly by developers today.
>     
>     Only the trace_event code should use the tracepoint interface
>     directly and no new tracepoints should be added.
>     
>     Besides, the example had a race condition with the use of the
>      ->d_name.name dentry field, as pointed out by Al Viro.
>     
>     Best just to remove the code so it wont be used by other developers.
>     
>     Cc: Al Viro <viro@ZenIV.linux.org.uk>
>     Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/samples/Kconfig b/samples/Kconfig
> index 7b6792a..6181c2c 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -5,12 +5,6 @@ menuconfig SAMPLES
>  
>  if SAMPLES
>  
> -config SAMPLE_TRACEPOINTS
> -	tristate "Build tracepoints examples -- loadable modules only"
> -	depends on TRACEPOINTS && m
> -	help
> -	  This build tracepoints example modules.
> -
>  config SAMPLE_TRACE_EVENTS
>  	tristate "Build trace_events examples -- loadable modules only"
>  	depends on EVENT_TRACING && m
> diff --git a/samples/Makefile b/samples/Makefile
> index 5ef08bb..1a60c62 100644
> --- a/samples/Makefile
> +++ b/samples/Makefile
> @@ -1,4 +1,4 @@
>  # Makefile for Linux samples code
>  
> -obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/ \
> +obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ trace_events/ \
>  			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
> diff --git a/samples/tracepoints/Makefile b/samples/tracepoints/Makefile
> deleted file mode 100644
> index 36479ad..0000000
> --- a/samples/tracepoints/Makefile
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -# builds the tracepoint example kernel modules;
> -# then to use one (as root):  insmod <module_name.ko>
> -
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-sample.o
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample.o
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample2.o
> diff --git a/samples/tracepoints/tp-samples-trace.h b/samples/tracepoints/tp-samples-trace.h
> deleted file mode 100644
> index 4d46be9..0000000
> --- a/samples/tracepoints/tp-samples-trace.h
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -#ifndef _TP_SAMPLES_TRACE_H
> -#define _TP_SAMPLES_TRACE_H
> -
> -#include <linux/proc_fs.h>	/* for struct inode and struct file */
> -#include <linux/tracepoint.h>
> -
> -DECLARE_TRACE(subsys_event,
> -	TP_PROTO(struct inode *inode, struct file *file),
> -	TP_ARGS(inode, file));
> -DECLARE_TRACE_NOARGS(subsys_eventb);
> -#endif
> diff --git a/samples/tracepoints/tracepoint-probe-sample.c b/samples/tracepoints/tracepoint-probe-sample.c
> deleted file mode 100644
> index 744c0b9..0000000
> --- a/samples/tracepoints/tracepoint-probe-sample.c
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -/*
> - * tracepoint-probe-sample.c
> - *
> - * sample tracepoint probes.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/file.h>
> -#include <linux/dcache.h>
> -#include "tp-samples-trace.h"
> -
> -/*
> - * Here the caller only guarantees locking for struct file and struct inode.
> - * Locking must therefore be done in the probe to use the dentry.
> - */
> -static void probe_subsys_event(void *ignore,
> -			       struct inode *inode, struct file *file)
> -{
> -	path_get(&file->f_path);
> -	dget(file->f_path.dentry);
> -	printk(KERN_INFO "Event is encountered with filename %s\n",
> -		file->f_path.dentry->d_name.name);
> -	dput(file->f_path.dentry);
> -	path_put(&file->f_path);
> -}
> -
> -static void probe_subsys_eventb(void *ignore)
> -{
> -	printk(KERN_INFO "Event B is encountered\n");
> -}
> -
> -static int __init tp_sample_trace_init(void)
> -{
> -	int ret;
> -
> -	ret = register_trace_subsys_event(probe_subsys_event, NULL);
> -	WARN_ON(ret);
> -	ret = register_trace_subsys_eventb(probe_subsys_eventb, NULL);
> -	WARN_ON(ret);
> -
> -	return 0;
> -}
> -
> -module_init(tp_sample_trace_init);
> -
> -static void __exit tp_sample_trace_exit(void)
> -{
> -	unregister_trace_subsys_eventb(probe_subsys_eventb, NULL);
> -	unregister_trace_subsys_event(probe_subsys_event, NULL);
> -	tracepoint_synchronize_unregister();
> -}
> -
> -module_exit(tp_sample_trace_exit);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint Probes Samples");
> diff --git a/samples/tracepoints/tracepoint-probe-sample2.c b/samples/tracepoints/tracepoint-probe-sample2.c
> deleted file mode 100644
> index 9fcf990..0000000
> --- a/samples/tracepoints/tracepoint-probe-sample2.c
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -/*
> - * tracepoint-probe-sample2.c
> - *
> - * 2nd sample tracepoint probes.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/fs.h>
> -#include "tp-samples-trace.h"
> -
> -/*
> - * Here the caller only guarantees locking for struct file and struct inode.
> - * Locking must therefore be done in the probe to use the dentry.
> - */
> -static void probe_subsys_event(void *ignore,
> -			       struct inode *inode, struct file *file)
> -{
> -	printk(KERN_INFO "Event is encountered with inode number %lu\n",
> -		inode->i_ino);
> -}
> -
> -static int __init tp_sample_trace_init(void)
> -{
> -	int ret;
> -
> -	ret = register_trace_subsys_event(probe_subsys_event, NULL);
> -	WARN_ON(ret);
> -
> -	return 0;
> -}
> -
> -module_init(tp_sample_trace_init);
> -
> -static void __exit tp_sample_trace_exit(void)
> -{
> -	unregister_trace_subsys_event(probe_subsys_event, NULL);
> -	tracepoint_synchronize_unregister();
> -}
> -
> -module_exit(tp_sample_trace_exit);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint Probes Samples");
> diff --git a/samples/tracepoints/tracepoint-sample.c b/samples/tracepoints/tracepoint-sample.c
> deleted file mode 100644
> index f4d89e0..0000000
> --- a/samples/tracepoints/tracepoint-sample.c
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -/* tracepoint-sample.c
> - *
> - * Executes a tracepoint when /proc/tracepoint-sample is opened.
> - *
> - * (C) Copyright 2007 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> - *
> - * This file is released under the GPLv2.
> - * See the file COPYING for more details.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/sched.h>
> -#include <linux/proc_fs.h>
> -#include "tp-samples-trace.h"
> -
> -DEFINE_TRACE(subsys_event);
> -DEFINE_TRACE(subsys_eventb);
> -
> -struct proc_dir_entry *pentry_sample;
> -
> -static int my_open(struct inode *inode, struct file *file)
> -{
> -	int i;
> -
> -	trace_subsys_event(inode, file);
> -	for (i = 0; i < 10; i++)
> -		trace_subsys_eventb();
> -	return -EPERM;
> -}
> -
> -static const struct file_operations mark_ops = {
> -	.open = my_open,
> -	.llseek = noop_llseek,
> -};
> -
> -static int __init sample_init(void)
> -{
> -	printk(KERN_ALERT "sample init\n");
> -	pentry_sample = proc_create("tracepoint-sample", 0444, NULL,
> -		&mark_ops);
> -	if (!pentry_sample)
> -		return -EPERM;
> -	return 0;
> -}
> -
> -static void __exit sample_exit(void)
> -{
> -	printk(KERN_ALERT "sample exit\n");
> -	remove_proc_entry("tracepoint-sample", NULL);
> -}
> -
> -module_init(sample_init)
> -module_exit(sample_exit)
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint sample");
> 
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [tracepoint] cargo-culting considered harmful...

[Mathieu Desnoyers]

* Al Viro (viro@ZenIV.linux.org.uk) wrote:
> On Fri, Jan 25, 2013 at 09:49:53AM -0500, Mathieu Desnoyers wrote:
> > static
> > void lttng_enumerate_task_fd(struct lttng_session *session,
> >                 struct task_struct *p, char *tmp)
> > {
> >         struct fdtable *fdt;
> >         struct file *filp;
> >         unsigned int i;
> >         const unsigned char *path;
> > 
> >         task_lock(p);
> >         if (!p->files)
> >                 goto unlock_task;
> >         spin_lock(&p->files->file_lock);
> >         fdt = files_fdtable(p->files);
> >         for (i = 0; i < fdt->max_fds; i++) {
> >                 filp = fcheck_files(p->files, i);
> >                 if (!filp)
> >                         continue;
> >                 path = d_path(&filp->f_path, tmp, PAGE_SIZE);
> >                 /* Make sure we give at least some info */
> >                 trace_lttng_statedump_file_descriptor(session, p, i,
> >                         IS_ERR(path) ?
> >                                 filp->f_dentry->d_name.name :
> >                                 path);
> >         }
> >         spin_unlock(&p->files->file_lock);
> > unlock_task:
> >         task_unlock(p);
> > }
> 
> *cringe*
> 
> a) yes, it needs d_lock for that ->d_name access
> b) iterate_fd() is there for purpose; use it, instead of open-coding the
> damn loop.  Something like
> 
> struct ctx {
> 	char *page;
> 	struct lttng_session *session,
> 	struct task_struct *p;
> };
> 	
> static int dump_one(void *p, struct file *file, unsigned fd)
> {
> 	struct ctx *ctx = p;
> 	const char *s = d_path(&file->f_path, ctx->page, PAGE_SIZE);
> 	struct dentry *dentry;
> 	if (!IS_ERR(s)) {
> 		trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd, s);
> 		return 0;
> 	}
> 	/* Make sure we give at least some info */
> 	dentry = file->f_path.dentry;
> 	spin_lock(&dentry->d_lock);
> 	trace_lttng_statedump_file_descriptor(ctx->session, ctx->p, fd,
> 		dentry->d_name);
> 	spin_unlock(&dentry->d_lock);
> 	return 0;
> }
> 
> ...
> 	task_lock(p);
> 	iterate_fd(p->files, 0, dump_one, &(struct ctx){tmp, session, p});
> 	task_unlock(p);
> 
> assuming it wouldn't be better to pass tmp/session/p as the single pointer
> to struct in the first place - I don't know enough about the callers of
> that sucker to tell.  And yes, iterate_fd() will DTRT if given NULL as the
> first argument.  The second argument is "which descriptor should I start
> from?", callback is called for everything present in the table starting from
> that place until it returns non-zero or the end of table is reached...

Thanks !! Modulo a couple of trivial nits, I've integrated your
suggestions. I'm creating a lttng_iterate_fd() wrapper for older kernels
(yeah.. we deal with kernels back to 2.6.32).

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[tip:perf/core] tracing: Remove tracepoint sample code

[tip-bot for Steven Rostedt]

Commit-ID:  d75f717e19fe595e7efbf67de195ada8d89dfbbe
Gitweb:     http://git.kernel.org/tip/d75f717e19fe595e7efbf67de195ada8d89dfbbe
Author:     Steven Rostedt <srostedt@redhat.com>
AuthorDate: Fri, 25 Jan 2013 09:46:36 -0500
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 25 Jan 2013 11:22:11 -0500

tracing: Remove tracepoint sample code

The tracepoint sample code was used to teach developers how to
create their own tracepoints. But now the trace_events have been
added as a higher level that is used directly by developers today.

Only the trace_event code should use the tracepoint interface
directly and no new tracepoints should be added.

Besides, the example had a race condition with the use of the
 ->d_name.name dentry field, as pointed out by Al Viro.

Best just to remove the code so it wont be used by other developers.

Link: http://lkml.kernel.org/r/20130123225523.GY4939@ZenIV.linux.org.uk

Cc: Al Viro <viro@ZenIV.linux.org.uk>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 samples/Kconfig                                |  6 ---
 samples/Makefile                               |  2 +-
 samples/tracepoints/Makefile                   |  6 ---
 samples/tracepoints/tp-samples-trace.h         | 11 -----
 samples/tracepoints/tracepoint-probe-sample.c  | 57 --------------------------
 samples/tracepoints/tracepoint-probe-sample2.c | 44 --------------------
 samples/tracepoints/tracepoint-sample.c        | 57 --------------------------
 7 files changed, 1 insertion(+), 182 deletions(-)

diff --git a/samples/Kconfig b/samples/Kconfig
index 7b6792a..6181c2c 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -5,12 +5,6 @@ menuconfig SAMPLES
 
 if SAMPLES
 
-config SAMPLE_TRACEPOINTS
-	tristate "Build tracepoints examples -- loadable modules only"
-	depends on TRACEPOINTS && m
-	help
-	  This build tracepoints example modules.
-
 config SAMPLE_TRACE_EVENTS
 	tristate "Build trace_events examples -- loadable modules only"
 	depends on EVENT_TRACING && m
diff --git a/samples/Makefile b/samples/Makefile
index 5ef08bb..1a60c62 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -1,4 +1,4 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/ \
+obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ trace_events/ \
 			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
diff --git a/samples/tracepoints/Makefile b/samples/tracepoints/Makefile
deleted file mode 100644
index 36479ad..0000000
--- a/samples/tracepoints/Makefile
+++ /dev/null
@@ -1,6 +0,0 @@
-# builds the tracepoint example kernel modules;
-# then to use one (as root):  insmod <module_name.ko>
-
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-sample.o
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample.o
-obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample2.o
diff --git a/samples/tracepoints/tp-samples-trace.h b/samples/tracepoints/tp-samples-trace.h
deleted file mode 100644
index 4d46be9..0000000
--- a/samples/tracepoints/tp-samples-trace.h
+++ /dev/null
@@ -1,11 +0,0 @@
-#ifndef _TP_SAMPLES_TRACE_H
-#define _TP_SAMPLES_TRACE_H
-
-#include <linux/proc_fs.h>	/* for struct inode and struct file */
-#include <linux/tracepoint.h>
-
-DECLARE_TRACE(subsys_event,
-	TP_PROTO(struct inode *inode, struct file *file),
-	TP_ARGS(inode, file));
-DECLARE_TRACE_NOARGS(subsys_eventb);
-#endif
diff --git a/samples/tracepoints/tracepoint-probe-sample.c b/samples/tracepoints/tracepoint-probe-sample.c
deleted file mode 100644
index 744c0b9..0000000
--- a/samples/tracepoints/tracepoint-probe-sample.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*
- * tracepoint-probe-sample.c
- *
- * sample tracepoint probes.
- */
-
-#include <linux/module.h>
-#include <linux/file.h>
-#include <linux/dcache.h>
-#include "tp-samples-trace.h"
-
-/*
- * Here the caller only guarantees locking for struct file and struct inode.
- * Locking must therefore be done in the probe to use the dentry.
- */
-static void probe_subsys_event(void *ignore,
-			       struct inode *inode, struct file *file)
-{
-	path_get(&file->f_path);
-	dget(file->f_path.dentry);
-	printk(KERN_INFO "Event is encountered with filename %s\n",
-		file->f_path.dentry->d_name.name);
-	dput(file->f_path.dentry);
-	path_put(&file->f_path);
-}
-
-static void probe_subsys_eventb(void *ignore)
-{
-	printk(KERN_INFO "Event B is encountered\n");
-}
-
-static int __init tp_sample_trace_init(void)
-{
-	int ret;
-
-	ret = register_trace_subsys_event(probe_subsys_event, NULL);
-	WARN_ON(ret);
-	ret = register_trace_subsys_eventb(probe_subsys_eventb, NULL);
-	WARN_ON(ret);
-
-	return 0;
-}
-
-module_init(tp_sample_trace_init);
-
-static void __exit tp_sample_trace_exit(void)
-{
-	unregister_trace_subsys_eventb(probe_subsys_eventb, NULL);
-	unregister_trace_subsys_event(probe_subsys_event, NULL);
-	tracepoint_synchronize_unregister();
-}
-
-module_exit(tp_sample_trace_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint Probes Samples");
diff --git a/samples/tracepoints/tracepoint-probe-sample2.c b/samples/tracepoints/tracepoint-probe-sample2.c
deleted file mode 100644
index 9fcf990..0000000
--- a/samples/tracepoints/tracepoint-probe-sample2.c
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * tracepoint-probe-sample2.c
- *
- * 2nd sample tracepoint probes.
- */
-
-#include <linux/module.h>
-#include <linux/fs.h>
-#include "tp-samples-trace.h"
-
-/*
- * Here the caller only guarantees locking for struct file and struct inode.
- * Locking must therefore be done in the probe to use the dentry.
- */
-static void probe_subsys_event(void *ignore,
-			       struct inode *inode, struct file *file)
-{
-	printk(KERN_INFO "Event is encountered with inode number %lu\n",
-		inode->i_ino);
-}
-
-static int __init tp_sample_trace_init(void)
-{
-	int ret;
-
-	ret = register_trace_subsys_event(probe_subsys_event, NULL);
-	WARN_ON(ret);
-
-	return 0;
-}
-
-module_init(tp_sample_trace_init);
-
-static void __exit tp_sample_trace_exit(void)
-{
-	unregister_trace_subsys_event(probe_subsys_event, NULL);
-	tracepoint_synchronize_unregister();
-}
-
-module_exit(tp_sample_trace_exit);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint Probes Samples");
diff --git a/samples/tracepoints/tracepoint-sample.c b/samples/tracepoints/tracepoint-sample.c
deleted file mode 100644
index f4d89e0..0000000
--- a/samples/tracepoints/tracepoint-sample.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/* tracepoint-sample.c
- *
- * Executes a tracepoint when /proc/tracepoint-sample is opened.
- *
- * (C) Copyright 2007 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
- *
- * This file is released under the GPLv2.
- * See the file COPYING for more details.
- */
-
-#include <linux/module.h>
-#include <linux/sched.h>
-#include <linux/proc_fs.h>
-#include "tp-samples-trace.h"
-
-DEFINE_TRACE(subsys_event);
-DEFINE_TRACE(subsys_eventb);
-
-struct proc_dir_entry *pentry_sample;
-
-static int my_open(struct inode *inode, struct file *file)
-{
-	int i;
-
-	trace_subsys_event(inode, file);
-	for (i = 0; i < 10; i++)
-		trace_subsys_eventb();
-	return -EPERM;
-}
-
-static const struct file_operations mark_ops = {
-	.open = my_open,
-	.llseek = noop_llseek,
-};
-
-static int __init sample_init(void)
-{
-	printk(KERN_ALERT "sample init\n");
-	pentry_sample = proc_create("tracepoint-sample", 0444, NULL,
-		&mark_ops);
-	if (!pentry_sample)
-		return -EPERM;
-	return 0;
-}
-
-static void __exit sample_exit(void)
-{
-	printk(KERN_ALERT "sample exit\n");
-	remove_proc_entry("tracepoint-sample", NULL);
-}
-
-module_init(sample_init)
-module_exit(sample_exit)
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Mathieu Desnoyers");
-MODULE_DESCRIPTION("Tracepoint sample");