2020-01-01 09:36:21

by Wen Yang

Subject: [PATCH] ftrace: avoid potential division by zero

The ftrace_profile->counter is unsigned long and
do_div truncates it to 32 bits, which means it can test
non-zero and be truncated to zero for division.
Fix this issue by using div64_ul() instead.

Signed-off-by: Wen Yang <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
---
kernel/trace/ftrace.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index ac99a35..a490ba5 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -527,7 +527,7 @@ static int function_stat_show(struct seq_file *m, void *v)

#ifdef CONFIG_FUNCTION_GRAPH_TRACER
avg = rec->time;
- do_div(avg, rec->counter);
+ avg = div64_ul(avg, rec->counter);
if (tracing_thresh && (avg < tracing_thresh))
goto out;
#endif
@@ -553,7 +553,8 @@ static int function_stat_show(struct seq_file *m, void *v)
* Divide only 1000 for ns^2 -> us^2 conversion.
* trace_print_graph_duration will divide 1000 again.
*/
- do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
+ stddev = div64_ul(stddev,
+ rec->counter * (rec->counter - 1) * 1000);
}

trace_seq_init(&s);
--
1.8.3.1

2020-01-02 15:44:33

by Steven Rostedt

Subject: Re: [PATCH] ftrace: avoid potential division by zero

On Wed, 1 Jan 2020 17:32:19 +0800
Wen Yang <[email protected]> wrote:

> The ftrace_profile->counter is unsigned long and
> do_div truncates it to 32 bits, which means it can test
> non-zero and be truncated to zero for division.
> Fix this issue by using div64_ul() instead.

Thanks, but since we are using div64_ul() which has different semantics
than do_div() let's clean up the code that was written to deal with the
strange do_div() semantics.

>
> Signed-off-by: Wen Yang <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> ---
> kernel/trace/ftrace.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index ac99a35..a490ba5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -527,7 +527,7 @@ static int function_stat_show(struct seq_file *m, void *v)
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> avg = rec->time;
> - do_div(avg, rec->counter);
> + avg = div64_ul(avg, rec->counter);

The above should be:

avg = div64_ul(rec->time, rec->counter);

and get rid of the pre-assigning of avg.


> if (tracing_thresh && (avg < tracing_thresh))
> goto out;
> #endif
> @@ -553,7 +553,8 @@ static int function_stat_show(struct seq_file *m, void *v)
> * Divide only 1000 for ns^2 -> us^2 conversion.
> * trace_print_graph_duration will divide 1000 again.
> */
> - do_div(stddev, rec->counter * (rec->counter - 1) * 1000);
> + stddev = div64_ul(stddev,
> + rec->counter * (rec->counter - 1) * 1000);

This can stay as is, because of the complex dividend in the equation.

Thanks,

-- Steve


> }
>
> trace_seq_init(&s);