[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