* [tracepoint] cargo-culting considered harmful...
@ 2013-01-23 22:55 Al Viro
2013-01-23 23:02 ` Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Al Viro @ 2013-01-23 22:55 UTC (permalink / raw)
To: Mathieu Desnoyers; +Cc: linux-kernel, Linus Torvalds
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [tracepoint] cargo-culting considered harmful... 2013-01-23 22:55 [tracepoint] cargo-culting considered harmful Al Viro @ 2013-01-23 23:02 ` Steven Rostedt 2013-01-25 14:38 ` Mathieu Desnoyers 2013-01-23 23:51 ` Andrew Morton 2013-02-03 19:16 ` [tip:perf/core] tracing: Remove tracepoint sample code tip-bot for Steven Rostedt 2 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2013-01-23 23:02 UTC (permalink / raw) To: Al Viro; +Cc: Mathieu Desnoyers, linux-kernel, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-23 23:02 ` Steven Rostedt @ 2013-01-25 14:38 ` Mathieu Desnoyers 2013-01-25 15:27 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2013-01-25 14:38 UTC (permalink / raw) To: Steven Rostedt; +Cc: Al Viro, linux-kernel, Linus Torvalds * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-25 14:38 ` Mathieu Desnoyers @ 2013-01-25 15:27 ` Steven Rostedt 2013-01-25 16:14 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2013-01-25 15:27 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Al Viro, linux-kernel, Linus Torvalds, Andrew Morton 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"); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-25 15:27 ` Steven Rostedt @ 2013-01-25 16:14 ` Mathieu Desnoyers 0 siblings, 0 replies; 11+ messages in thread From: Mathieu Desnoyers @ 2013-01-25 16:14 UTC (permalink / raw) To: Steven Rostedt; +Cc: Al Viro, linux-kernel, Linus Torvalds, Andrew Morton * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-23 22:55 [tracepoint] cargo-culting considered harmful Al Viro 2013-01-23 23:02 ` Steven Rostedt @ 2013-01-23 23:51 ` Andrew Morton 2013-01-24 1:48 ` Al Viro 2013-02-03 19:16 ` [tip:perf/core] tracing: Remove tracepoint sample code tip-bot for Steven Rostedt 2 siblings, 1 reply; 11+ messages in thread From: Andrew Morton @ 2013-01-23 23:51 UTC (permalink / raw) To: Al Viro; +Cc: Mathieu Desnoyers, linux-kernel, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-23 23:51 ` Andrew Morton @ 2013-01-24 1:48 ` Al Viro 2013-01-25 14:49 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Al Viro @ 2013-01-24 1:48 UTC (permalink / raw) To: Andrew Morton; +Cc: Mathieu Desnoyers, linux-kernel, Linus Torvalds 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 ;-/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-24 1:48 ` Al Viro @ 2013-01-25 14:49 ` Mathieu Desnoyers 2013-01-25 15:32 ` Al Viro 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2013-01-25 14:49 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, linux-kernel, Linus Torvalds * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-25 14:49 ` Mathieu Desnoyers @ 2013-01-25 15:32 ` Al Viro 2013-01-25 17:30 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Al Viro @ 2013-01-25 15:32 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Andrew Morton, linux-kernel, Linus Torvalds 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 ;-) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [tracepoint] cargo-culting considered harmful... 2013-01-25 15:32 ` Al Viro @ 2013-01-25 17:30 ` Mathieu Desnoyers 0 siblings, 0 replies; 11+ messages in thread From: Mathieu Desnoyers @ 2013-01-25 17:30 UTC (permalink / raw) To: Al Viro; +Cc: linux-kernel, lttng-dev * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip:perf/core] tracing: Remove tracepoint sample code 2013-01-23 22:55 [tracepoint] cargo-culting considered harmful Al Viro 2013-01-23 23:02 ` Steven Rostedt 2013-01-23 23:51 ` Andrew Morton @ 2013-02-03 19:16 ` tip-bot for Steven Rostedt 2 siblings, 0 replies; 11+ messages in thread From: tip-bot for Steven Rostedt @ 2013-02-03 19:16 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, mathieu.desnoyers, viro, rostedt, srostedt, tglx 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"); ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-02-03 19:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-01-23 22:55 [tracepoint] cargo-culting considered harmful Al Viro 2013-01-23 23:02 ` Steven Rostedt 2013-01-25 14:38 ` Mathieu Desnoyers 2013-01-25 15:27 ` Steven Rostedt 2013-01-25 16:14 ` Mathieu Desnoyers 2013-01-23 23:51 ` Andrew Morton 2013-01-24 1:48 ` Al Viro 2013-01-25 14:49 ` Mathieu Desnoyers 2013-01-25 15:32 ` Al Viro 2013-01-25 17:30 ` Mathieu Desnoyers 2013-02-03 19:16 ` [tip:perf/core] tracing: Remove tracepoint sample code tip-bot for Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).