From mboxrd@z Thu Jan  1 00:00:00 1970
From: compudj@krystal.dyndns.org (Mathieu Desnoyers)
Date: Mon, 23 Feb 2009 01:50:59 -0500
Subject: [ltt-dev] [PATCH] lttng: Fix potential invalid
	pointer	derefference
In-Reply-To: <49A2452C.9000808@cn.fujitsu.com>
References: <49A237E5.4010001@cn.fujitsu.com> <20090223055955.GA11351@Krystal>
	<49A2452C.9000808@cn.fujitsu.com>
Message-ID: <20090223065059.GA13598@Krystal>

* 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