From mboxrd@z Thu Jan  1 00:00:00 1970
From: compudj@krystal.dyndns.org (Mathieu Desnoyers)
Date: Mon, 23 Feb 2009 09:37:09 -0500
Subject: [ltt-dev] [PATCH v2] lttng: Fix potential invalid
	pointer	derefference
In-Reply-To: <49A26C0B.3010005@cn.fujitsu.com>
References: <49A237E5.4010001@cn.fujitsu.com> <49A26C0B.3010005@cn.fujitsu.com>
Message-ID: <20090223143709.GA27239@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.
> 
> 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