(2015/01/15 0:40), Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rost...@goodmis.org>
>
> Using just the filter for checking for trampolines or regs is not enough
> when updating the code against the records that represent all functions.
> Both the filter hash and the notrace hash need to be checked.
>
> To trigger this bug (using trace-cmd and perf):
>
> # perf probe -a do_fork
> # trace-cmd start -B foo -e probe
> # trace-cmd record -p function_graph -n do_fork sleep 1
>
> The trace-cmd record at the end clears the filter before it disables
> function_graph tracing and then that causes the accounting of the
> ftrace function records to become incorrect and causes ftrace to bug.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Steven Rostedt <rost...@goodmis.org>
> ---
> kernel/trace/ftrace.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 2b35d0ba578d..224e768bdc73 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2497,12 +2497,14 @@ static void ftrace_run_update_code(int command)
> }
>
> static void ftrace_run_modify_code(struct ftrace_ops *ops, int command,
> - struct ftrace_hash *old_hash)
> + struct ftrace_ops_hash *old_hash)
> {
> ops->flags |= FTRACE_OPS_FL_MODIFYING;
> - ops->old_hash.filter_hash = old_hash;
> + ops->old_hash.filter_hash = old_hash->filter_hash;
> + ops->old_hash.notrace_hash = old_hash->notrace_hash;
> ftrace_run_update_code(command);
> ops->old_hash.filter_hash = NULL;
> + ops->old_hash.notrace_hash = NULL;
> ops->flags &= ~FTRACE_OPS_FL_MODIFYING;
> }
>
> @@ -3579,7 +3581,7 @@ static struct ftrace_ops trace_probe_ops __read_mostly =
>
> static int ftrace_probe_registered;
>
> -static void __enable_ftrace_function_probe(struct ftrace_hash *old_hash)
> +static void __enable_ftrace_function_probe(struct ftrace_ops_hash *old_hash)
> {
> int ret;
> int i;
> @@ -3637,6 +3639,7 @@ int
> register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
> void *data)
> {
> + struct ftrace_ops_hash old_hash_ops;
Would it be better to be old_ops_hash? since it's not an ops.
Other parts are OK for me :)
Reviewed-by: Masami Hiramatsu <masami.hiramatsu...@hitachi.com>
Thank you,
> struct ftrace_func_probe *entry;
> struct ftrace_hash **orig_hash =
> &trace_probe_ops.func_hash->filter_hash;
> struct ftrace_hash *old_hash = *orig_hash;
> @@ -3658,6 +3661,10 @@ register_ftrace_function_probe(char *glob, struct
> ftrace_probe_ops *ops,
>
> mutex_lock(&trace_probe_ops.func_hash->regex_lock);
>
> + old_hash_ops.filter_hash = old_hash;
> + /* Probes only have filters */
> + old_hash_ops.notrace_hash = NULL;
> +
> hash = alloc_and_copy_ftrace_hash(FTRACE_HASH_DEFAULT_BITS, old_hash);
> if (!hash) {
> count = -ENOMEM;
> @@ -3718,7 +3725,7 @@ register_ftrace_function_probe(char *glob, struct
> ftrace_probe_ops *ops,
>
> ret = ftrace_hash_move(&trace_probe_ops, 1, orig_hash, hash);
>
> - __enable_ftrace_function_probe(old_hash);
> + __enable_ftrace_function_probe(&old_hash_ops);
>
> if (!ret)
> free_ftrace_hash_rcu(old_hash);
> @@ -4006,7 +4013,7 @@ ftrace_match_addr(struct ftrace_hash *hash, unsigned
> long ip, int remove)
> }
>
> static void ftrace_ops_update_code(struct ftrace_ops *ops,
> - struct ftrace_hash *old_hash)
> + struct ftrace_ops_hash *old_hash)
> {
> struct ftrace_ops *op;
>
> @@ -4041,6 +4048,7 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char
> *buf, int len,
> unsigned long ip, int remove, int reset, int enable)
> {
> struct ftrace_hash **orig_hash;
> + struct ftrace_ops_hash old_hash_ops;
> struct ftrace_hash *old_hash;
> struct ftrace_hash *hash;
> int ret;
> @@ -4077,9 +4085,11 @@ ftrace_set_hash(struct ftrace_ops *ops, unsigned char
> *buf, int len,
>
> mutex_lock(&ftrace_lock);
> old_hash = *orig_hash;
> + old_hash_ops.filter_hash = ops->func_hash->filter_hash;
> + old_hash_ops.notrace_hash = ops->func_hash->notrace_hash;
> ret = ftrace_hash_move(ops, enable, orig_hash, hash);
> if (!ret) {
> - ftrace_ops_update_code(ops, old_hash);
> + ftrace_ops_update_code(ops, &old_hash_ops);
> free_ftrace_hash_rcu(old_hash);
> }
> mutex_unlock(&ftrace_lock);
> @@ -4291,6 +4301,7 @@ static void __init set_ftrace_early_filters(void)
> int ftrace_regex_release(struct inode *inode, struct file *file)
> {
> struct seq_file *m = (struct seq_file *)file->private_data;
> + struct ftrace_ops_hash old_hash_ops;
> struct ftrace_iterator *iter;
> struct ftrace_hash **orig_hash;
> struct ftrace_hash *old_hash;
> @@ -4324,10 +4335,12 @@ int ftrace_regex_release(struct inode *inode, struct
> file *file)
>
> mutex_lock(&ftrace_lock);
> old_hash = *orig_hash;
> + old_hash_ops.filter_hash = iter->ops->func_hash->filter_hash;
> + old_hash_ops.notrace_hash = iter->ops->func_hash->notrace_hash;
> ret = ftrace_hash_move(iter->ops, filter_hash,
> orig_hash, iter->hash);
> if (!ret) {
> - ftrace_ops_update_code(iter->ops, old_hash);
> + ftrace_ops_update_code(iter->ops, &old_hash_ops);
> free_ftrace_hash_rcu(old_hash);
> }
> mutex_unlock(&ftrace_lock);
> -- 2.1.4
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
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/
