[ltt-dev] [PATCH] lttng: Fix potential invalid pointer derefference
Mathieu Desnoyers
compudj at krystal.dyndns.org
Mon Feb 23 00:59:56 EST 2009
* 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(<t_root_dentry->d_subdirs)) {
> - debugfs_remove(ltt_root_dentry);
> - ltt_root_dentry = NULL;
> + if (ltt_root) {
> + if (atomic_dec_and_test(<t_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(<t_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(<t_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, <t_setup_trace_operations);
> + S_IWUSR, ltt_root->root, NULL, <t_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, <t_destroy_trace_operations);
> + S_IWUSR, ltt_root->root, NULL, <t_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,
> <t_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
More information about the lttng-dev
mailing list