[PATCH v3] tracepoints: prevents null probe from being added

[PATCH v3] tracepoints: prevents null probe from being added

[kpark3469]

From: Sahara <keun-o.park@windriver.com>

Somehow tracepoint_entry_add_probe function allows a null probe function.
And, this may lead to unexpected result since the number of probe
functions in an entry can be counted by checking whether probe is null
or not in for-loop.
This patch prevents the null probe from being added.
In tracepoint_entry_remove_probe function, checking probe parameter
within for-loop is moved out for code efficiency leaving the null probe
feature which removes all probe functions in the entry.

Signed-off-by: Sahara <keun-o.park@windriver.com>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 kernel/tracepoint.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 0c05a45..29f2654 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
 	int nr_probes = 0;
 	struct tracepoint_func *old, *new;
 
-	WARN_ON(!probe);
+	if (WARN_ON(!probe))
+		return ERR_PTR(-EINVAL);
 
 	debug_print_probes(entry);
 	old = entry->funcs;
@@ -152,13 +153,18 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
 
 	debug_print_probes(entry);
 	/* (N -> M), (N > 1, M >= 0) probes */
-	for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
-		if (!probe ||
-		    (old[nr_probes].func == probe &&
-		     old[nr_probes].data == data))
-			nr_del++;
+	if (probe) {
+		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+			if (old[nr_probes].func == probe &&
+			     old[nr_probes].data == data)
+				nr_del++;
+		}
 	}
 
+	/*
+	 * If probe is NULL, then nr_probes = nr_del = 0, and then the
+	 * entire entry will be removed.
+	 */
 	if (nr_probes - nr_del == 0) {
 		/* N -> 0, (N > 1) */
 		entry->funcs = NULL;
@@ -173,8 +179,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
 		if (new == NULL)
 			return ERR_PTR(-ENOMEM);
 		for (i = 0; old[i].func; i++)
-			if (probe &&
-			    (old[i].func != probe || old[i].data != data))
+			if (old[i].func != probe || old[i].data != data)
 				new[j++] = old[i];
 		new[nr_probes - nr_del].func = NULL;
 		entry->refcount = nr_probes - nr_del;
-- 
1.7.1

Re: [PATCH v3] tracepoints: prevents null probe from being added

[Steven Rostedt]

On Mon, 2013-04-15 at 11:13 +0900, kpark3469@gmail.com wrote:
> From: Sahara <keun-o.park@windriver.com>
> 
> Somehow tracepoint_entry_add_probe function allows a null probe function.
> And, this may lead to unexpected result since the number of probe
> functions in an entry can be counted by checking whether probe is null
> or not in for-loop.
> This patch prevents the null probe from being added.
> In tracepoint_entry_remove_probe function, checking probe parameter
> within for-loop is moved out for code efficiency leaving the null probe
> feature which removes all probe functions in the entry.
> 
> Signed-off-by: Sahara <keun-o.park@windriver.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

BTW, do not add tags that were not given to you. "Reviewed-by" has a
meaning, more than just someone that reviewed your patch. It means that
they not only reviewed your patch but couldn't find anything wrong with
it. As both Mathieu and I had comments, that does not deserve a
"Reviewed-by" tag.

I'm not even sure that Mathieu gave an "Acked-by". I thought he did, but
I can't seem to find it. Mathieu?

Anyway, I'll start testing this patch as it seems fine with me (although
I still wouldn't give a Reviewed-by tag).

Thanks,

-- Steve

> ---
>  kernel/tracepoint.c |   21 +++++++++++++--------
>  1 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0c05a45..29f2654 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
>  	int nr_probes = 0;
>  	struct tracepoint_func *old, *new;
>  
> -	WARN_ON(!probe);
> +	if (WARN_ON(!probe))
> +		return ERR_PTR(-EINVAL);
>  
>  	debug_print_probes(entry);
>  	old = entry->funcs;
> @@ -152,13 +153,18 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
>  
>  	debug_print_probes(entry);
>  	/* (N -> M), (N > 1, M >= 0) probes */
> -	for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> -		if (!probe ||
> -		    (old[nr_probes].func == probe &&
> -		     old[nr_probes].data == data))
> -			nr_del++;
> +	if (probe) {
> +		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> +			if (old[nr_probes].func == probe &&
> +			     old[nr_probes].data == data)
> +				nr_del++;
> +		}
>  	}
>  
> +	/*
> +	 * If probe is NULL, then nr_probes = nr_del = 0, and then the
> +	 * entire entry will be removed.
> +	 */
>  	if (nr_probes - nr_del == 0) {
>  		/* N -> 0, (N > 1) */
>  		entry->funcs = NULL;
> @@ -173,8 +179,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
>  		if (new == NULL)
>  			return ERR_PTR(-ENOMEM);
>  		for (i = 0; old[i].func; i++)
> -			if (probe &&
> -			    (old[i].func != probe || old[i].data != data))
> +			if (old[i].func != probe || old[i].data != data)
>  				new[j++] = old[i];
>  		new[nr_probes - nr_del].func = NULL;
>  		entry->refcount = nr_probes - nr_del;

Re: [PATCH v3] tracepoints: prevents null probe from being added

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Mon, 2013-04-15 at 11:13 +0900, kpark3469@gmail.com wrote:
> > From: Sahara <keun-o.park@windriver.com>
> > 
> > Somehow tracepoint_entry_add_probe function allows a null probe function.
> > And, this may lead to unexpected result since the number of probe
> > functions in an entry can be counted by checking whether probe is null
> > or not in for-loop.
> > This patch prevents the null probe from being added.
> > In tracepoint_entry_remove_probe function, checking probe parameter
> > within for-loop is moved out for code efficiency leaving the null probe
> > feature which removes all probe functions in the entry.
> > 
> > Signed-off-by: Sahara <keun-o.park@windriver.com>
> > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> BTW, do not add tags that were not given to you. "Reviewed-by" has a
> meaning, more than just someone that reviewed your patch. It means that
> they not only reviewed your patch but couldn't find anything wrong with
> it. As both Mathieu and I had comments, that does not deserve a
> "Reviewed-by" tag.
> 
> I'm not even sure that Mathieu gave an "Acked-by". I thought he did, but
> I can't seem to find it. Mathieu?

I don't recall, but all my comments were addressed. In order to clear
any confusion:

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> 
> Anyway, I'll start testing this patch as it seems fine with me (although
> I still wouldn't give a Reviewed-by tag).
> 
> Thanks,
> 
> -- Steve
> 
> > ---
> >  kernel/tracepoint.c |   21 +++++++++++++--------
> >  1 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 0c05a45..29f2654 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
> >  	int nr_probes = 0;
> >  	struct tracepoint_func *old, *new;
> >  
> > -	WARN_ON(!probe);
> > +	if (WARN_ON(!probe))
> > +		return ERR_PTR(-EINVAL);
> >  
> >  	debug_print_probes(entry);
> >  	old = entry->funcs;
> > @@ -152,13 +153,18 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
> >  
> >  	debug_print_probes(entry);
> >  	/* (N -> M), (N > 1, M >= 0) probes */
> > -	for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > -		if (!probe ||
> > -		    (old[nr_probes].func == probe &&
> > -		     old[nr_probes].data == data))
> > -			nr_del++;
> > +	if (probe) {
> > +		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > +			if (old[nr_probes].func == probe &&
> > +			     old[nr_probes].data == data)
> > +				nr_del++;
> > +		}
> >  	}
> >  
> > +	/*
> > +	 * If probe is NULL, then nr_probes = nr_del = 0, and then the
> > +	 * entire entry will be removed.
> > +	 */
> >  	if (nr_probes - nr_del == 0) {
> >  		/* N -> 0, (N > 1) */
> >  		entry->funcs = NULL;
> > @@ -173,8 +179,7 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry,
> >  		if (new == NULL)
> >  			return ERR_PTR(-ENOMEM);
> >  		for (i = 0; old[i].func; i++)
> > -			if (probe &&
> > -			    (old[i].func != probe || old[i].data != data))
> > +			if (old[i].func != probe || old[i].data != data)
> >  				new[j++] = old[i];
> >  		new[nr_probes - nr_del].func = NULL;
> >  		entry->refcount = nr_probes - nr_del;
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com