Re: [PATCH 2/2] ftrace: setup correct flags before replace code of module rec

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 28 Jul 2020 18:27:20 +0800
Chengming Zhou <zhouchengming@bytedance.com> wrote:

> When module loaded and enabled, we will use __ftrace_replace_code
> for module if any ftrace_ops referenced it found. But we will get
> wrong ftrace_addr for module rec in ftrace_get_addr_new, because
> rec->flags has not been setup correctly.
> So setup correct rec->flags when we call referenced_filters to find
> ftrace_ops references it.

This is somewhat correct ;-)

> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  kernel/trace/ftrace.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index fca01a168ae5..00087dea0174 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6190,8 +6190,17 @@ static int referenced_filters(struct dyn_ftrace *rec)
>  	int cnt = 0;
>  
>  	for (ops = ftrace_ops_list; ops != &ftrace_list_end; ops = ops->next) {
> -		if (ops_references_rec(ops, rec))
> -		    cnt++;
> +		if (ops_references_rec(ops, rec)) {

> +			cnt++;
> +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> +				rec->flags |= FTRACE_FL_DIRECT;

The above should be:

			if (WARN_ON_ONCE(ops->flags & FTRACE_OPS_FL_DIRECT))
				continue;
			cnt++;

The direct flag is *very* special, and should not be set automatically
like this.

Probably should add the same kind of warning and skip for
FTRACE_OPS_FL_IPMODIFY.

> +			if (ops->flags & FTRACE_OPS_FL_SAVE_REGS)
> +				rec->flags |= FTRACE_FL_REGS;

The above is definitely a bug fix. I'm thinking this patch should be
broken up into two. One with just this update (and the clear below),
and the rest later. As this should be backported to stable.

> +			if (cnt == 1 && ops->trampoline)
> +				rec->flags |= FTRACE_FL_TRAMP;
> +			else
> +				rec->flags &= ~FTRACE_FL_TRAMP;

The above is correct, but not critical that it would need to be
backported.

Thanks!

-- Steve

> +		}
>  	}
>  
>  	return cnt;
> @@ -6373,7 +6382,8 @@ void ftrace_module_enable(struct module *mod)
>  			cnt += referenced_filters(rec);
>  
>  		/* This clears FTRACE_FL_DISABLED */
> -		rec->flags = cnt;
> +		rec->flags &= ~FTRACE_FL_DISABLED;
> +		rec->flags += cnt;
>  
>  		if (ftrace_start_up && cnt) {
>  			int failed = __ftrace_replace_code(rec, 1);