Re: [PATCH 2/2] tracing: fix referencing after memory freeing andrefactors code

[Steven Rostedt]

Sorry for the late review, I was busy getting ready for kernel summit
and then traveling too much.


On Fri, 18 Oct 2013 17:59:42 -0300
"Geyslan G. Bem" <geyslan@xxxxxxxxx> wrote:

> In 'system_tr_open()':
> Fix possible 'dir' assignment after freeing it.
> 
> In both functions:
> Restructures logic conditions testing 'tracing_is_disabled()'
> return before the others tests.
> Centralizes the exiting in accordance to Coding Style, Chapter 7.
> 
> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx>
> ---
>  kernel/trace/trace_events.c | 46 +++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 368a4d5..0f56ebf 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>  	struct event_subsystem *system = NULL;
>  	struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
>  	struct trace_array *tr;
> -	int ret;
> +	int ret = -ENODEV;
> +
> +	if (tracing_is_disabled())
> +		return ret;
>  
>  	/* Make sure the system still exists */
>  	mutex_lock(&trace_types_lock);
> @@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode, struct file *filp)
>  	mutex_unlock(&trace_types_lock);
>  
>  	if (!system)
> -		return -ENODEV;
> +		return ret;
>  
>  	/* Some versions of gcc think dir can be uninitialized here */
>  	WARN_ON(!dir);
>  
>  	/* Still need to increment the ref count of the system */
> -	if (trace_array_get(tr) < 0) {
> -		put_system(dir);
> -		return -ENODEV;
> -	}
> +	ret = trace_array_get(tr);
> +	if (ret)
> +		goto err_get;
>  
> -	ret = tracing_open_generic(inode, filp);
> -	if (ret < 0) {
> -		trace_array_put(tr);
> -		put_system(dir);
> -	}
> +	filp->private_data = dir;
> +	return 0;
>  
> +err_get:
> +	put_system(dir);
>  	return ret;

I don't see any improvement in the above code, except for the initial
check of tracing_is_disabled(). Just add:

	if (tracing_is_disabled())
		return -ENODEV;

no need for all the fancy work with using ret and goto.


>  }
>  
> @@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode, struct file *filp)
>  {
>  	struct ftrace_subsystem_dir *dir;
>  	struct trace_array *tr = inode->i_private;
> -	int ret;
> +	int ret = -ENODEV;
>  
> -	if (trace_array_get(tr) < 0)
> -		return -ENODEV;
> +	if (tracing_is_disabled())
> +		return ret;
> +
> +	ret = trace_array_get(tr);
> +	if (ret)
> +		return ret;
>  
>  	/* Make a temporary dir that has no system but points to tr */
>  	dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>  	if (!dir) {
> -		trace_array_put(tr);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_dir;
>  	}
>  
>  	dir->tr = tr;
>  
> -	ret = tracing_open_generic(inode, filp);
> -	if (ret < 0) {
> -		trace_array_put(tr);
> -		kfree(dir);
> -	}
> -
>  	filp->private_data = dir;
> +	return 0;
>  

Again, we don't need the goto and err_dir. The simple fix is to finish
the function with:

	ret = tracing_open_generic(inode, filp);
	if (ret < 0) {
		trace_array_put(tr);
		kfree(dir);
		return ret;
	}

	filp->private_data = dir;

	return 0;

-- Steve

> +err_dir:
> +	trace_array_put(tr);
>  	return ret;
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/