From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Mon, 23 Feb 2009 00:59:56 -0500 Subject: [ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference In-Reply-To: <49A237E5.4010001@cn.fujitsu.com> References: <49A237E5.4010001@cn.fujitsu.com> Message-ID: <20090223055955.GA11351@Krystal> * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote: > Hi Mathieu, > > Calling get_ltt_root() and put_ltt_root() pair may induce invalid pointer > derefference. Consider the following scenario: > > CPU 0 CPU 1 > ---------------------- ---------------------- > 1 root = get_ltt_root() > 2 root = get_ltt_root() > 3 put_ltt_root() //global root is freed > 4 using root(crash here) > > Here is the fix for this issue. Thanks a lot to Zhaolei to point this out > in offline. > Agreed, there is a problem. Please see comments below, > This patch assumes that the *ltt root remove* patch has been applied. > > Signed-off-by: Gui Jianfeng > --- > include/linux/ltt-core.h | 8 +++++++- > ltt/ltt-core.c | 38 +++++++++++++++++++++++++++----------- Only modifications to the two files above seems justified. The rest could probably stay as-is. > ltt/ltt-relay.c | 7 ++++++- > ltt/ltt-trace-control.c | 13 +++++++------ > ltt/ltt-userspace-event.c | 12 ++++++++---- > 5 files changed, 55 insertions(+), 23 deletions(-) > > diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h > index 28394cb..4e20f78 100644 > --- a/include/linux/ltt-core.h > +++ b/include/linux/ltt-core.h > @@ -27,11 +27,17 @@ struct ltt_traces { > > extern struct ltt_traces ltt_traces; > > +/* ltt's root dir */ > +struct ltt_root { > + atomic_t ref; > + struct dentry *root; > +}; > + Please use include/linux/kref.h. > /* > * get dentry of ltt's root dir > */ > +struct ltt_root *get_ltt_root(void); > void put_ltt_root(void); > -struct dentry *get_ltt_root(void); > You should not export the "ref". It can be dealt with internally by get/put. Creating a struct ltt_root is pointless. > /* Keep track of trap nesting inside LTT */ > DECLARE_PER_CPU(unsigned int, ltt_nesting); > diff --git a/ltt/ltt-core.c b/ltt/ltt-core.c > index 314750b..ed393bd 100644 > --- a/ltt/ltt-core.c > +++ b/ltt/ltt-core.c > @@ -6,10 +6,10 @@ > * Distributed under the GPL license > */ > > -#include > #include > #include > #include > +#include > Don't move headers around. Thanks, Mathieu > /* Traces structures */ > struct ltt_traces ltt_traces = { > @@ -21,26 +21,42 @@ EXPORT_SYMBOL(ltt_traces); > /* Traces list writer locking */ > static DEFINE_MUTEX(ltt_traces_mutex); > > -/* dentry of ltt's root dir */ > -static struct dentry *ltt_root_dentry; > +static struct ltt_root *ltt_root; > > void put_ltt_root(void) > { > - if (ltt_root_dentry && list_empty(<t_root_dentry->d_subdirs)) { > - debugfs_remove(ltt_root_dentry); > - ltt_root_dentry = NULL; > + if (ltt_root) { > + if (atomic_dec_and_test(<t_root->ref)) { > + debugfs_remove(ltt_root->root); > + kfree(ltt_root); > + ltt_root = NULL; > + } > } > } > EXPORT_SYMBOL_GPL(put_ltt_root); > > -struct dentry *get_ltt_root(void) > +struct ltt_root *get_ltt_root(void) > { > - if (!ltt_root_dentry) { > - ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL); > - if (!ltt_root_dentry) > + if (!ltt_root) { > + ltt_root = kmalloc(sizeof(*ltt_root), GFP_KERNEL); > + if (!ltt_root) > + return NULL; > + atomic_set(<t_root->ref, 1); > + ltt_root->root = debugfs_create_dir(LTT_ROOT, NULL); > + if (!ltt_root->root) { > printk(KERN_ERR "LTT : create ltt root dir failed\n"); > + goto create_dir_fail; > + } > + return ltt_root; > + } else { > + atomic_inc(<t_root->ref); > + return ltt_root; > } > - return ltt_root_dentry; > + > +create_dir_fail: > + kfree(ltt_root); > + ltt_root = NULL; > + return NULL; > } > EXPORT_SYMBOL_GPL(get_ltt_root); > > diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c > index 7c544f4..6e4e4e5 100644 > --- a/ltt/ltt-relay.c > +++ b/ltt/ltt-relay.c > @@ -833,8 +833,13 @@ end: > > static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace) > { > + struct ltt_root *ltt_root; > + > + ltt_root = get_ltt_root(); > new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name, > - get_ltt_root()); > + ltt_root->root); > + put_ltt_root(); > + ltt_root = NULL; > if (new_trace->dentry.trace_root == NULL) { > printk(KERN_ERR "LTT : Trace directory name %s already taken\n", > new_trace->trace_name); > diff --git a/ltt/ltt-trace-control.c b/ltt/ltt-trace-control.c > index d7c9119..aa40e71 100644 > --- a/ltt/ltt-trace-control.c > +++ b/ltt/ltt-trace-control.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > > #define LTT_CONTROL_DIR "control" > #define LTT_SETUP_TRACE_FILE "setup_trace" > @@ -696,15 +697,15 @@ static struct file_operations ltt_destroy_trace_operations = { > static int __init ltt_trace_control_init(void) > { > int err = 0; > - struct dentry *ltt_root_dentry; > + struct ltt_root *ltt_root; > > - ltt_root_dentry = get_ltt_root(); > - if (!ltt_root_dentry) { > + ltt_root = get_ltt_root(); > + if (!ltt_root) { > err = -ENOENT; > goto err_no_root; > } > > - ltt_control_dir = debugfs_create_dir(LTT_CONTROL_DIR, ltt_root_dentry); > + ltt_control_dir = debugfs_create_dir(LTT_CONTROL_DIR, ltt_root->root); > if (IS_ERR(ltt_control_dir) || !ltt_control_dir) { > printk(KERN_ERR > "ltt_channel_control_init: create dir of %s failed\n", > @@ -714,7 +715,7 @@ static int __init ltt_trace_control_init(void) > } > > ltt_setup_trace_file = debugfs_create_file(LTT_SETUP_TRACE_FILE, > - S_IWUSR, ltt_root_dentry, NULL, <t_setup_trace_operations); > + S_IWUSR, ltt_root->root, NULL, <t_setup_trace_operations); > if (IS_ERR(ltt_setup_trace_file) || !ltt_setup_trace_file) { > printk(KERN_ERR > "ltt_channel_control_init: create file of %s failed\n", > @@ -724,7 +725,7 @@ static int __init ltt_trace_control_init(void) > } > > ltt_destroy_trace_file = debugfs_create_file(LTT_DESTROY_TRACE_FILE, > - S_IWUSR, ltt_root_dentry, NULL, <t_destroy_trace_operations); > + S_IWUSR, ltt_root->root, NULL, <t_destroy_trace_operations); > if (IS_ERR(ltt_destroy_trace_file) || !ltt_destroy_trace_file) { > printk(KERN_ERR > "ltt_channel_control_init: create file of %s failed\n", > diff --git a/ltt/ltt-userspace-event.c b/ltt/ltt-userspace-event.c > index a48f336..5686022 100644 > --- a/ltt/ltt-userspace-event.c > +++ b/ltt/ltt-userspace-event.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > > #include "probes/ltt-type-serializer.h" > > @@ -89,18 +90,18 @@ static struct file_operations ltt_userspace_operations = { > > static int __init ltt_userspace_init(void) > { > - struct dentry *ltt_root_dentry; > + struct ltt_root *ltt_root; > int err = 0; > > - ltt_root_dentry = get_ltt_root(); > - if (!ltt_root_dentry) { > + ltt_root = get_ltt_root(); > + if (!ltt_root) { > err = -ENOENT; > goto err_no_root; > } > > ltt_event_file = debugfs_create_file(LTT_WRITE_EVENT_FILE, > S_IWUGO, > - ltt_root_dentry, > + ltt_root->root, > NULL, > <t_userspace_operations); > if (IS_ERR(ltt_event_file) || !ltt_event_file) { > @@ -110,7 +111,10 @@ static int __init ltt_userspace_init(void) > err = -EPERM; > goto err_no_file; > } > + > + return err; > err_no_file: > + put_ltt_root(); > err_no_root: > return err; > } > -- > 1.5.4.rc3 > > > > _______________________________________________ > ltt-dev mailing list > ltt-dev at lists.casi.polymtl.ca > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68