* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.