[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference

[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

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.

This patch assumes that the *ltt root remove* patch has been applied.

Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
---
 include/linux/ltt-core.h  |    8 +++++++-
 ltt/ltt-core.c            |   38 +++++++++++++++++++++++++++-----------
 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;
+};
+
 /*
  * 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);
 
 /* 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 <linux/ltt-core.h>
 #include <linux/percpu.h>
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/ltt-core.h>
 
 /* 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(&ltt_root_dentry->d_subdirs)) {
-		debugfs_remove(ltt_root_dentry);
-		ltt_root_dentry = NULL;
+	if (ltt_root) {
+		if (atomic_dec_and_test(&ltt_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(&ltt_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(&ltt_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 <linux/uaccess.h>
 #include <linux/debugfs.h>
 #include <linux/ltt-tracer.h>
+#include <linux/ltt-core.h>
 
 #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, &ltt_setup_trace_operations);
+		S_IWUSR, ltt_root->root, NULL, &ltt_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, &ltt_destroy_trace_operations);
+		S_IWUSR, ltt_root->root, NULL, &ltt_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 <linux/gfp.h>
 #include <linux/fs.h>
 #include <linux/debugfs.h>
+#include <linux/ltt-core.h>
 
 #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,
 					     &ltt_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] [PATCH] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* 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 <guijianfeng at cn.fujistu.com>
> ---
>  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 <linux/ltt-core.h>
>  #include <linux/percpu.h>
>  #include <linux/module.h>
>  #include <linux/debugfs.h>
> +#include <linux/ltt-core.h>
>  

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(&ltt_root_dentry->d_subdirs)) {
> -		debugfs_remove(ltt_root_dentry);
> -		ltt_root_dentry = NULL;
> +	if (ltt_root) {
> +		if (atomic_dec_and_test(&ltt_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(&ltt_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(&ltt_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 <linux/uaccess.h>
>  #include <linux/debugfs.h>
>  #include <linux/ltt-tracer.h>
> +#include <linux/ltt-core.h>
>  
>  #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, &ltt_setup_trace_operations);
> +		S_IWUSR, ltt_root->root, NULL, &ltt_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, &ltt_destroy_trace_operations);
> +		S_IWUSR, ltt_root->root, NULL, &ltt_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 <linux/gfp.h>
>  #include <linux/fs.h>
>  #include <linux/debugfs.h>
> +#include <linux/ltt-core.h>
>  
>  #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,
>  					     &ltt_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

[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

Mathieu Desnoyers wrote:
> * 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 <guijianfeng at cn.fujistu.com>
>> ---
>>  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.

  Do you mean this struct should move into ltt-core.c as following:
  struct ltt_root {
	struct dentry *root;
	struct kref ref;
  };
  and get_ltt_root() will still return a dentry pointer?

> 
>>  /*
>>   * 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 <linux/ltt-core.h>
>>  #include <linux/percpu.h>
>>  #include <linux/module.h>
>>  #include <linux/debugfs.h>
>> +#include <linux/ltt-core.h>
>>  
> 
> Don't move headers around.
> 
> Thanks,
> 
> Mathieu


-- 
Regards
Gui Jianfeng

[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * 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 <guijianfeng at cn.fujistu.com>
> >> ---
> >>  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.
> 
>   Do you mean this struct should move into ltt-core.c as following:
>   struct ltt_root {
> 	struct dentry *root;
> 	struct kref ref;
>   };
>   and get_ltt_root() will still return a dentry pointer?
> 

Or even stay as

struct dentry *root;
struct kref root_dentry_kref;

There is no real need for a structure around them.

Mathieu

> > 
> >>  /*
> >>   * 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 <linux/ltt-core.h>
> >>  #include <linux/percpu.h>
> >>  #include <linux/module.h>
> >>  #include <linux/debugfs.h>
> >> +#include <linux/ltt-core.h>
> >>  
> > 
> > Don't move headers around.
> > 
> > Thanks,
> > 
> > Mathieu
> 
> 
> -- 
> Regards
> Gui Jianfeng
> 
> 
> _______________________________________________
> 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

[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

Mathieu Desnoyers wrote:
> * Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
>> Mathieu Desnoyers wrote:
>>> * 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 <guijianfeng at cn.fujistu.com>
>>>> ---
>>>>  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.
>>   Do you mean this struct should move into ltt-core.c as following:
>>   struct ltt_root {
>> 	struct dentry *root;
>> 	struct kref ref;
>>   };
>>   and get_ltt_root() will still return a dentry pointer?
>>
> 
> Or even stay as
> 
> struct dentry *root;
> struct kref root_dentry_kref;
> 
> There is no real need for a structure around them.

  See, will post v2

> 
> Mathieu


-- 
Regards
Gui Jianfeng

[ltt-dev] [PATCH v2] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

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.

This patch assumes that the *ltt root remove* patch has been applied.

Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
---
 include/linux/ltt-core.h  |    3 ++-
 ltt/ltt-core.c            |   25 +++++++++++++++++++++----
 ltt/ltt-relay.c           |   10 +++++++++-
 ltt/ltt-userspace-event.c |    3 +++
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
index 28394cb..786c2b1 100644
--- a/include/linux/ltt-core.h
+++ b/include/linux/ltt-core.h
@@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
 /*
  * get dentry of ltt's root dir
  */
-void put_ltt_root(void);
 struct dentry *get_ltt_root(void);
 
+void put_ltt_root(void);
+
 /* 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..03b81ca 100644
--- a/ltt/ltt-core.c
+++ b/ltt/ltt-core.c
@@ -10,6 +10,7 @@
 #include <linux/percpu.h>
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/kref.h>
 
 /* Traces structures */
 struct ltt_traces ltt_traces = {
@@ -23,12 +24,22 @@ static DEFINE_MUTEX(ltt_traces_mutex);
 
 /* dentry of ltt's root dir */
 static struct dentry *ltt_root_dentry;
+static struct kref ltt_root_kref = {
+	.refcount = ATOMIC_INIT(0),
+};
+
+static void ltt_root_release(struct kref *ref)
+{
+	return;
+}
 
 void put_ltt_root(void)
 {
-	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
-		debugfs_remove(ltt_root_dentry);
-		ltt_root_dentry = NULL;
+	if (ltt_root_dentry) {
+		if (kref_put(&ltt_root_kref, ltt_root_release)) {
+			debugfs_remove(ltt_root_dentry);
+			ltt_root_dentry = NULL;
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(put_ltt_root);
@@ -37,9 +48,15 @@ struct dentry *get_ltt_root(void)
 {
 	if (!ltt_root_dentry) {
 		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
-		if (!ltt_root_dentry)
+		if (!ltt_root_dentry) {
 			printk(KERN_ERR "LTT : create ltt root dir failed\n");
+			goto out;
+		}
+		kref_init(&ltt_root_kref);
+		goto out;
 	}
+	kref_get(&ltt_root_kref);
+out:
 	return ltt_root_dentry;
 }
 EXPORT_SYMBOL_GPL(get_ltt_root);
diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
index 7c544f4..8378802 100644
--- a/ltt/ltt-relay.c
+++ b/ltt/ltt-relay.c
@@ -833,8 +833,16 @@ end:
 
 static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
 {
+	struct dentry *ltt_root_dentry;
+
+	ltt_root_dentry = get_ltt_root();
+	if (!ltt_root_dentry)
+		return ENOENT;
+
 	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
-			get_ltt_root());
+			ltt_root_dentry);
+	put_ltt_root();
+	ltt_root_dentry = 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-userspace-event.c b/ltt/ltt-userspace-event.c
index a48f336..487516b 100644
--- a/ltt/ltt-userspace-event.c
+++ b/ltt/ltt-userspace-event.c
@@ -110,7 +110,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] [PATCH v2] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* 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.
> 
> This patch assumes that the *ltt root remove* patch has been applied.
> 
> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
> ---
>  include/linux/ltt-core.h  |    3 ++-
>  ltt/ltt-core.c            |   25 +++++++++++++++++++++----
>  ltt/ltt-relay.c           |   10 +++++++++-
>  ltt/ltt-userspace-event.c |    3 +++
>  4 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
> index 28394cb..786c2b1 100644
> --- a/include/linux/ltt-core.h
> +++ b/include/linux/ltt-core.h
> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
>  /*
>   * get dentry of ltt's root dir
>   */
> -void put_ltt_root(void);
>  struct dentry *get_ltt_root(void);
>  
> +void put_ltt_root(void);
> +
>  /* 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..03b81ca 100644
> --- a/ltt/ltt-core.c
> +++ b/ltt/ltt-core.c
> @@ -10,6 +10,7 @@
>  #include <linux/percpu.h>
>  #include <linux/module.h>
>  #include <linux/debugfs.h>
> +#include <linux/kref.h>
>  
>  /* Traces structures */
>  struct ltt_traces ltt_traces = {
> @@ -23,12 +24,22 @@ static DEFINE_MUTEX(ltt_traces_mutex);
>  
>  /* dentry of ltt's root dir */
>  static struct dentry *ltt_root_dentry;
> +static struct kref ltt_root_kref = {
> +	.refcount = ATOMIC_INIT(0),
> +};
> +
> +static void ltt_root_release(struct kref *ref)
> +{
> +	return;
> +}
>  
>  void put_ltt_root(void)
>  {
> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
> -		debugfs_remove(ltt_root_dentry);
> -		ltt_root_dentry = NULL;
> +	if (ltt_root_dentry) {
> +		if (kref_put(&ltt_root_kref, ltt_root_release)) {
> +			debugfs_remove(ltt_root_dentry);
> +			ltt_root_dentry = NULL;

Those two should go in ltt_root_release. See other (wide) use of
kref_get/kref_put in the LTT code as example.

Mathieu

> +		}
>  	}
>  }
>  EXPORT_SYMBOL_GPL(put_ltt_root);
> @@ -37,9 +48,15 @@ struct dentry *get_ltt_root(void)
>  {
>  	if (!ltt_root_dentry) {
>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
> -		if (!ltt_root_dentry)
> +		if (!ltt_root_dentry) {
>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
> +			goto out;
> +		}
> +		kref_init(&ltt_root_kref);
> +		goto out;
>  	}
> +	kref_get(&ltt_root_kref);
> +out:
>  	return ltt_root_dentry;
>  }
>  EXPORT_SYMBOL_GPL(get_ltt_root);
> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> index 7c544f4..8378802 100644
> --- a/ltt/ltt-relay.c
> +++ b/ltt/ltt-relay.c
> @@ -833,8 +833,16 @@ end:
>  
>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
>  {
> +	struct dentry *ltt_root_dentry;
> +
> +	ltt_root_dentry = get_ltt_root();
> +	if (!ltt_root_dentry)
> +		return ENOENT;
> +
>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
> -			get_ltt_root());
> +			ltt_root_dentry);
> +	put_ltt_root();
> +	ltt_root_dentry = NULL;

If ltt_root_dentry is local to the function, I don't see real value in
setting it to null ?

You should also modifify ltt-relay-locked, because it has almost the
exact same code as ltt-relay.c.

Thanks,

Mathieu

>  	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-userspace-event.c b/ltt/ltt-userspace-event.c
> index a48f336..487516b 100644
> --- a/ltt/ltt-userspace-event.c
> +++ b/ltt/ltt-userspace-event.c
> @@ -110,7 +110,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

[ltt-dev] [PATCH v2] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

Mathieu Desnoyers wrote:
> * 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.
>>
>> This patch assumes that the *ltt root remove* patch has been applied.
>>
>> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
>> ---
>>  include/linux/ltt-core.h  |    3 ++-
>>  ltt/ltt-core.c            |   25 +++++++++++++++++++++----
>>  ltt/ltt-relay.c           |   10 +++++++++-
>>  ltt/ltt-userspace-event.c |    3 +++
>>  4 files changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
>> index 28394cb..786c2b1 100644
>> --- a/include/linux/ltt-core.h
>> +++ b/include/linux/ltt-core.h
>> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
>>  /*
>>   * get dentry of ltt's root dir
>>   */
>> -void put_ltt_root(void);
>>  struct dentry *get_ltt_root(void);
>>  
>> +void put_ltt_root(void);
>> +
>>  /* 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..03b81ca 100644
>> --- a/ltt/ltt-core.c
>> +++ b/ltt/ltt-core.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/percpu.h>
>>  #include <linux/module.h>
>>  #include <linux/debugfs.h>
>> +#include <linux/kref.h>
>>  
>>  /* Traces structures */
>>  struct ltt_traces ltt_traces = {
>> @@ -23,12 +24,22 @@ static DEFINE_MUTEX(ltt_traces_mutex);
>>  
>>  /* dentry of ltt's root dir */
>>  static struct dentry *ltt_root_dentry;
>> +static struct kref ltt_root_kref = {
>> +	.refcount = ATOMIC_INIT(0),
>> +};
>> +
>> +static void ltt_root_release(struct kref *ref)
>> +{
>> +	return;
>> +}
>>  
>>  void put_ltt_root(void)
>>  {
>> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
>> -		debugfs_remove(ltt_root_dentry);
>> -		ltt_root_dentry = NULL;
>> +	if (ltt_root_dentry) {
>> +		if (kref_put(&ltt_root_kref, ltt_root_release)) {
>> +			debugfs_remove(ltt_root_dentry);
>> +			ltt_root_dentry = NULL;
> 
> Those two should go in ltt_root_release. See other (wide) use of
> kref_get/kref_put in the LTT code as example.
> 
> Mathieu
> 
>> +		}
>>  	}
>>  }
>>  EXPORT_SYMBOL_GPL(put_ltt_root);
>> @@ -37,9 +48,15 @@ struct dentry *get_ltt_root(void)
>>  {
>>  	if (!ltt_root_dentry) {
>>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
>> -		if (!ltt_root_dentry)
>> +		if (!ltt_root_dentry) {
>>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
>> +			goto out;
>> +		}
>> +		kref_init(&ltt_root_kref);
>> +		goto out;
>>  	}
>> +	kref_get(&ltt_root_kref);
>> +out:
>>  	return ltt_root_dentry;
>>  }
>>  EXPORT_SYMBOL_GPL(get_ltt_root);
>> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
>> index 7c544f4..8378802 100644
>> --- a/ltt/ltt-relay.c
>> +++ b/ltt/ltt-relay.c
>> @@ -833,8 +833,16 @@ end:
>>  
>>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
>>  {
>> +	struct dentry *ltt_root_dentry;
>> +
>> +	ltt_root_dentry = get_ltt_root();
>> +	if (!ltt_root_dentry)
>> +		return ENOENT;
>> +
>>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
>> -			get_ltt_root());
>> +			ltt_root_dentry);
>> +	put_ltt_root();
>> +	ltt_root_dentry = NULL;
> 
> If ltt_root_dentry is local to the function, I don't see real value in
> setting it to null ?

  Ok, will remove.

> 
> You should also modifify ltt-relay-locked, because it has almost the
> exact same code as ltt-relay.c.

  ltt-relay-locked doesn't use get_ltt_root(), it has its own root directory
  called "ltt-locked", so i don't see why ltt-relay-locked should be modified.

> 
> Thanks,
> 
> Mathieu
> 
>>  	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-userspace-event.c b/ltt/ltt-userspace-event.c
>> index a48f336..487516b 100644
>> --- a/ltt/ltt-userspace-event.c
>> +++ b/ltt/ltt-userspace-event.c
>> @@ -110,7 +110,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
>>
> 

-- 
Regards
Gui Jianfeng

[ltt-dev] [PATCH v2] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* Gui Jianfeng (guijianfeng at cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * 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.
> >>
> >> This patch assumes that the *ltt root remove* patch has been applied.
> >>
> >> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
> >> ---
> >>  include/linux/ltt-core.h  |    3 ++-
> >>  ltt/ltt-core.c            |   25 +++++++++++++++++++++----
> >>  ltt/ltt-relay.c           |   10 +++++++++-
> >>  ltt/ltt-userspace-event.c |    3 +++
> >>  4 files changed, 35 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
> >> index 28394cb..786c2b1 100644
> >> --- a/include/linux/ltt-core.h
> >> +++ b/include/linux/ltt-core.h
> >> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
> >>  /*
> >>   * get dentry of ltt's root dir
> >>   */
> >> -void put_ltt_root(void);
> >>  struct dentry *get_ltt_root(void);
> >>  
> >> +void put_ltt_root(void);
> >> +
> >>  /* 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..03b81ca 100644
> >> --- a/ltt/ltt-core.c
> >> +++ b/ltt/ltt-core.c
> >> @@ -10,6 +10,7 @@
> >>  #include <linux/percpu.h>
> >>  #include <linux/module.h>
> >>  #include <linux/debugfs.h>
> >> +#include <linux/kref.h>
> >>  
> >>  /* Traces structures */
> >>  struct ltt_traces ltt_traces = {
> >> @@ -23,12 +24,22 @@ static DEFINE_MUTEX(ltt_traces_mutex);
> >>  
> >>  /* dentry of ltt's root dir */
> >>  static struct dentry *ltt_root_dentry;
> >> +static struct kref ltt_root_kref = {
> >> +	.refcount = ATOMIC_INIT(0),
> >> +};
> >> +
> >> +static void ltt_root_release(struct kref *ref)
> >> +{
> >> +	return;
> >> +}
> >>  
> >>  void put_ltt_root(void)
> >>  {
> >> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
> >> -		debugfs_remove(ltt_root_dentry);
> >> -		ltt_root_dentry = NULL;
> >> +	if (ltt_root_dentry) {
> >> +		if (kref_put(&ltt_root_kref, ltt_root_release)) {
> >> +			debugfs_remove(ltt_root_dentry);
> >> +			ltt_root_dentry = NULL;
> > 
> > Those two should go in ltt_root_release. See other (wide) use of
> > kref_get/kref_put in the LTT code as example.
> > 
> > Mathieu
> > 
> >> +		}
> >>  	}
> >>  }
> >>  EXPORT_SYMBOL_GPL(put_ltt_root);
> >> @@ -37,9 +48,15 @@ struct dentry *get_ltt_root(void)
> >>  {
> >>  	if (!ltt_root_dentry) {
> >>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
> >> -		if (!ltt_root_dentry)
> >> +		if (!ltt_root_dentry) {
> >>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
> >> +			goto out;
> >> +		}
> >> +		kref_init(&ltt_root_kref);
> >> +		goto out;
> >>  	}
> >> +	kref_get(&ltt_root_kref);
> >> +out:
> >>  	return ltt_root_dentry;
> >>  }
> >>  EXPORT_SYMBOL_GPL(get_ltt_root);
> >> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> >> index 7c544f4..8378802 100644
> >> --- a/ltt/ltt-relay.c
> >> +++ b/ltt/ltt-relay.c
> >> @@ -833,8 +833,16 @@ end:
> >>  
> >>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
> >>  {
> >> +	struct dentry *ltt_root_dentry;
> >> +
> >> +	ltt_root_dentry = get_ltt_root();
> >> +	if (!ltt_root_dentry)
> >> +		return ENOENT;
> >> +
> >>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
> >> -			get_ltt_root());
> >> +			ltt_root_dentry);
> >> +	put_ltt_root();
> >> +	ltt_root_dentry = NULL;
> > 
> > If ltt_root_dentry is local to the function, I don't see real value in
> > setting it to null ?
> 
>   Ok, will remove.
> 
> > 
> > You should also modifify ltt-relay-locked, because it has almost the
> > exact same code as ltt-relay.c.
> 
>   ltt-relay-locked doesn't use get_ltt_root(), it has its own root directory
>   called "ltt-locked", so i don't see why ltt-relay-locked should be modified.
> 

Yes, you are right.

Mathieu

> > 
> > Thanks,
> > 
> > Mathieu
> > 
> >>  	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-userspace-event.c b/ltt/ltt-userspace-event.c
> >> index a48f336..487516b 100644
> >> --- a/ltt/ltt-userspace-event.c
> >> +++ b/ltt/ltt-userspace-event.c
> >> @@ -110,7 +110,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
> >>
> > 
> 
> -- 
> Regards
> Gui Jianfeng
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[ltt-dev] [PATCH v3] lttng: Fix potential invalid pointer derefference

[Gui Jianfeng]

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.

This patch assumes that the *ltt root remove* patch has been applied.

Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
---
 include/linux/ltt-core.h  |    3 ++-
 ltt/ltt-core.c            |   23 +++++++++++++++++++----
 ltt/ltt-relay.c           |    9 ++++++++-
 ltt/ltt-userspace-event.c |    3 +++
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
index 28394cb..786c2b1 100644
--- a/include/linux/ltt-core.h
+++ b/include/linux/ltt-core.h
@@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
 /*
  * get dentry of ltt's root dir
  */
-void put_ltt_root(void);
 struct dentry *get_ltt_root(void);
 
+void put_ltt_root(void);
+
 /* 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..52b2a83 100644
--- a/ltt/ltt-core.c
+++ b/ltt/ltt-core.c
@@ -10,6 +10,7 @@
 #include <linux/percpu.h>
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/kref.h>
 
 /* Traces structures */
 struct ltt_traces ltt_traces = {
@@ -23,12 +24,20 @@ static DEFINE_MUTEX(ltt_traces_mutex);
 
 /* dentry of ltt's root dir */
 static struct dentry *ltt_root_dentry;
+static struct kref ltt_root_kref = {
+	.refcount = ATOMIC_INIT(0),
+};
+
+static void ltt_root_release(struct kref *ref)
+{
+	debugfs_remove(ltt_root_dentry);
+	ltt_root_dentry = NULL;
+}
 
 void put_ltt_root(void)
 {
-	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
-		debugfs_remove(ltt_root_dentry);
-		ltt_root_dentry = NULL;
+	if (ltt_root_dentry) {
+		kref_put(&ltt_root_kref, ltt_root_release);
 	}
 }
 EXPORT_SYMBOL_GPL(put_ltt_root);
@@ -37,9 +46,15 @@ struct dentry *get_ltt_root(void)
 {
 	if (!ltt_root_dentry) {
 		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
-		if (!ltt_root_dentry)
+		if (!ltt_root_dentry) {
 			printk(KERN_ERR "LTT : create ltt root dir failed\n");
+			goto out;
+		}
+		kref_init(&ltt_root_kref);
+		goto out;
 	}
+	kref_get(&ltt_root_kref);
+out:
 	return ltt_root_dentry;
 }
 EXPORT_SYMBOL_GPL(get_ltt_root);
diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
index 7c544f4..55bb9d2 100644
--- a/ltt/ltt-relay.c
+++ b/ltt/ltt-relay.c
@@ -833,8 +833,15 @@ end:
 
 static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
 {
+	struct dentry *ltt_root_dentry;
+
+	ltt_root_dentry = get_ltt_root();
+	if (!ltt_root_dentry)
+		return ENOENT;
+
 	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
-			get_ltt_root());
+			ltt_root_dentry);
+	put_ltt_root();
 	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-userspace-event.c b/ltt/ltt-userspace-event.c
index a48f336..487516b 100644
--- a/ltt/ltt-userspace-event.c
+++ b/ltt/ltt-userspace-event.c
@@ -110,7 +110,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] [PATCH v3] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* 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.
> 
> This patch assumes that the *ltt root remove* patch has been applied.
> 

Perfect, will merge. Thanks !

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at polymtl.ca>

> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
> ---
>  include/linux/ltt-core.h  |    3 ++-
>  ltt/ltt-core.c            |   23 +++++++++++++++++++----
>  ltt/ltt-relay.c           |    9 ++++++++-
>  ltt/ltt-userspace-event.c |    3 +++
>  4 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
> index 28394cb..786c2b1 100644
> --- a/include/linux/ltt-core.h
> +++ b/include/linux/ltt-core.h
> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
>  /*
>   * get dentry of ltt's root dir
>   */
> -void put_ltt_root(void);
>  struct dentry *get_ltt_root(void);
>  
> +void put_ltt_root(void);
> +
>  /* 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..52b2a83 100644
> --- a/ltt/ltt-core.c
> +++ b/ltt/ltt-core.c
> @@ -10,6 +10,7 @@
>  #include <linux/percpu.h>
>  #include <linux/module.h>
>  #include <linux/debugfs.h>
> +#include <linux/kref.h>
>  
>  /* Traces structures */
>  struct ltt_traces ltt_traces = {
> @@ -23,12 +24,20 @@ static DEFINE_MUTEX(ltt_traces_mutex);
>  
>  /* dentry of ltt's root dir */
>  static struct dentry *ltt_root_dentry;
> +static struct kref ltt_root_kref = {
> +	.refcount = ATOMIC_INIT(0),
> +};
> +
> +static void ltt_root_release(struct kref *ref)
> +{
> +	debugfs_remove(ltt_root_dentry);
> +	ltt_root_dentry = NULL;
> +}
>  
>  void put_ltt_root(void)
>  {
> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
> -		debugfs_remove(ltt_root_dentry);
> -		ltt_root_dentry = NULL;
> +	if (ltt_root_dentry) {
> +		kref_put(&ltt_root_kref, ltt_root_release);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(put_ltt_root);
> @@ -37,9 +46,15 @@ struct dentry *get_ltt_root(void)
>  {
>  	if (!ltt_root_dentry) {
>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
> -		if (!ltt_root_dentry)
> +		if (!ltt_root_dentry) {
>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
> +			goto out;
> +		}
> +		kref_init(&ltt_root_kref);
> +		goto out;
>  	}
> +	kref_get(&ltt_root_kref);
> +out:
>  	return ltt_root_dentry;
>  }
>  EXPORT_SYMBOL_GPL(get_ltt_root);
> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> index 7c544f4..55bb9d2 100644
> --- a/ltt/ltt-relay.c
> +++ b/ltt/ltt-relay.c
> @@ -833,8 +833,15 @@ end:
>  
>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
>  {
> +	struct dentry *ltt_root_dentry;
> +
> +	ltt_root_dentry = get_ltt_root();
> +	if (!ltt_root_dentry)
> +		return ENOENT;
> +
>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
> -			get_ltt_root());
> +			ltt_root_dentry);
> +	put_ltt_root();
>  	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-userspace-event.c b/ltt/ltt-userspace-event.c
> index a48f336..487516b 100644
> --- a/ltt/ltt-userspace-event.c
> +++ b/ltt/ltt-userspace-event.c
> @@ -110,7 +110,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

[ltt-dev] [PATCH v3] lttng: Fix potential invalid pointer derefference

[Mathieu Desnoyers]

* 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.
> 
> This patch assumes that the *ltt root remove* patch has been applied.
> 

Small nit :


compudj at ok:~/git/morestable/linux-2.6-lttng$ scripts/checkpatch.pl patches/`quilt top`
WARNING: braces {} are not necessary for single statement blocks
#74: FILE: ltt/ltt-core.c:39:
+	if (ltt_root_dentry) {
+		kref_put(&ltt_root_kref, ltt_root_release);
 	}

total: 0 errors, 1 warnings, 83 lines checked

patches/lttng-remove-ltt-root-dir-fix.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

I'll fix it in my tree.

Mathieu

> Signed-off-by: Gui Jianfeng <guijianfeng at cn.fujistu.com>
> ---
>  include/linux/ltt-core.h  |    3 ++-
>  ltt/ltt-core.c            |   23 +++++++++++++++++++----
>  ltt/ltt-relay.c           |    9 ++++++++-
>  ltt/ltt-userspace-event.c |    3 +++
>  4 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ltt-core.h b/include/linux/ltt-core.h
> index 28394cb..786c2b1 100644
> --- a/include/linux/ltt-core.h
> +++ b/include/linux/ltt-core.h
> @@ -30,9 +30,10 @@ extern struct ltt_traces ltt_traces;
>  /*
>   * get dentry of ltt's root dir
>   */
> -void put_ltt_root(void);
>  struct dentry *get_ltt_root(void);
>  
> +void put_ltt_root(void);
> +
>  /* 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..52b2a83 100644
> --- a/ltt/ltt-core.c
> +++ b/ltt/ltt-core.c
> @@ -10,6 +10,7 @@
>  #include <linux/percpu.h>
>  #include <linux/module.h>
>  #include <linux/debugfs.h>
> +#include <linux/kref.h>
>  
>  /* Traces structures */
>  struct ltt_traces ltt_traces = {
> @@ -23,12 +24,20 @@ static DEFINE_MUTEX(ltt_traces_mutex);
>  
>  /* dentry of ltt's root dir */
>  static struct dentry *ltt_root_dentry;
> +static struct kref ltt_root_kref = {
> +	.refcount = ATOMIC_INIT(0),
> +};
> +
> +static void ltt_root_release(struct kref *ref)
> +{
> +	debugfs_remove(ltt_root_dentry);
> +	ltt_root_dentry = NULL;
> +}
>  
>  void put_ltt_root(void)
>  {
> -	if (ltt_root_dentry && list_empty(&ltt_root_dentry->d_subdirs)) {
> -		debugfs_remove(ltt_root_dentry);
> -		ltt_root_dentry = NULL;
> +	if (ltt_root_dentry) {
> +		kref_put(&ltt_root_kref, ltt_root_release);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(put_ltt_root);
> @@ -37,9 +46,15 @@ struct dentry *get_ltt_root(void)
>  {
>  	if (!ltt_root_dentry) {
>  		ltt_root_dentry = debugfs_create_dir(LTT_ROOT, NULL);
> -		if (!ltt_root_dentry)
> +		if (!ltt_root_dentry) {
>  			printk(KERN_ERR "LTT : create ltt root dir failed\n");
> +			goto out;
> +		}
> +		kref_init(&ltt_root_kref);
> +		goto out;
>  	}
> +	kref_get(&ltt_root_kref);
> +out:
>  	return ltt_root_dentry;
>  }
>  EXPORT_SYMBOL_GPL(get_ltt_root);
> diff --git a/ltt/ltt-relay.c b/ltt/ltt-relay.c
> index 7c544f4..55bb9d2 100644
> --- a/ltt/ltt-relay.c
> +++ b/ltt/ltt-relay.c
> @@ -833,8 +833,15 @@ end:
>  
>  static int ltt_relay_create_dirs(struct ltt_trace_struct *new_trace)
>  {
> +	struct dentry *ltt_root_dentry;
> +
> +	ltt_root_dentry = get_ltt_root();
> +	if (!ltt_root_dentry)
> +		return ENOENT;
> +
>  	new_trace->dentry.trace_root = debugfs_create_dir(new_trace->trace_name,
> -			get_ltt_root());
> +			ltt_root_dentry);
> +	put_ltt_root();
>  	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-userspace-event.c b/ltt/ltt-userspace-event.c
> index a48f336..487516b 100644
> --- a/ltt/ltt-userspace-event.c
> +++ b/ltt/ltt-userspace-event.c
> @@ -110,7 +110,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