Re: [PATCH v2] tracepoints: prevents null probe from being added
On Thu, 2013-03-21 at 14:34 +0900, kpark3...@gmail.com wrote:
> From: Sahara
>
> 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
> Reviewed-by: Steven Rostedt
> Reviewed-by: Mathieu Desnoyers
> ---
> kernel/tracepoint.c | 18 ++
> 1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0c05a45..7d69348 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,15 @@ 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, all funcs in the entry will be removed. */
OK, I first thought this was a bug as nr_del would be zero and not match
nr_probes, but then I realized that nr_probes would also be zero. Can
you update the above comment to say something like:
/*
* If probe is NULL, then nr_probes = nr_del = 0, and then the
* entire entry will be removed.
*/
Thanks,
-- Steve
> if (nr_probes - nr_del == 0) {
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> @@ -173,8 +176,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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] tracepoints: prevents null probe from being added
On Thu, Mar 21, 2013 at 2:34 PM, wrote:
> From: Sahara
>
> 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
> Reviewed-by: Steven Rostedt
> Reviewed-by: Mathieu Desnoyers
> ---
> kernel/tracepoint.c | 18 ++
> 1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 0c05a45..7d69348 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,15 @@ 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, all funcs in the entry will be removed. */
> if (nr_probes - nr_del == 0) {
> /* N -> 0, (N > 1) */
> entry->funcs = NULL;
> @@ -173,8 +176,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
>
Hi Steve,
Please check out this v2 patch. Seemingly I got no response from you.
Thanks.
-- Kpark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] tracepoints: prevents null probe from being added
From: Sahara
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
Reviewed-by: Steven Rostedt
Reviewed-by: Mathieu Desnoyers
---
kernel/tracepoint.c | 18 ++
1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 0c05a45..7d69348 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,15 @@ 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, all funcs in the entry will be removed. */
if (nr_probes - nr_del == 0) {
/* N -> 0, (N > 1) */
entry->funcs = NULL;
@@ -173,8 +176,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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
