Re: [PATCH 1/3] ftrace: Fix updating of filters for shared global_ops filters

[Masami Hiramatsu]

(2015/01/15 0:39), Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx>
> 
> As the set_ftrace_filter affects both the function tracer as well as the
> function graph tracer, the ops that represent each have a shared
> ftrace_ops_hash structure. This allows both to be updated when the filter
> files are updated.
> 
> But if function graph is enabled and the global_ops (function tracing) ops
> is not, then it is possible that the filter could be changed without the
> update happening for the function graph ops. This will cause the changes
> to not take place and may even cause a ftrace_bug to occur as it could mess
> with the trampoline accounting.
> 
> The solution is to check if the ops uses the shared global_ops filter and
> if the ops itself is not enabled, to check if there's another ops that is
> enabled and also shares the global_ops filter. In that case, the
> modification still needs to be executed.
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>

Thank you,

> Cc: stable@xxxxxxxxxxxxxxx # 3.17+
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
>  kernel/trace/ftrace.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 929a733d302e..2b35d0ba578d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4008,8 +4008,32 @@ 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)
>  {
> -	if (ops->flags & FTRACE_OPS_FL_ENABLED && ftrace_enabled)
> +	struct ftrace_ops *op;
> +
> +	if (!ftrace_enabled)
> +		return;
> +
> +	if (ops->flags & FTRACE_OPS_FL_ENABLED) {
>  		ftrace_run_modify_code(ops, FTRACE_UPDATE_CALLS, old_hash);
> +		return;
> +	}
> +
> +	/*
> +	 * If this is the shared global_ops filter, then we need to
> +	 * check if there is another ops that shares it, is enabled.
> +	 * If so, we still need to run the modify code.
> +	 */
> +	if (ops->func_hash != &global_ops.local_hash)
> +		return;
> +
> +	do_for_each_ftrace_op(op, ftrace_ops_list) {
> +		if (op->func_hash == &global_ops.local_hash &&
> +		    op->flags & FTRACE_OPS_FL_ENABLED) {
> +			ftrace_run_modify_code(op, FTRACE_UPDATE_CALLS, old_hash);
> +			/* Only need to do this once */
> +			return;
> +		}
> +	} while_for_each_ftrace_op(op);
>  }
>  
>  static int

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@xxxxxxxxxxx


--
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/