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

[guijianfeng@cn.fujitsu.com (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