Re: [PATCH v4] ftrace: Add FTRACE_MCOUNT_MAX_OFFSET to avoid adding weak function

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 26 May 2022 14:19:12 -0400
Steven Rostedt <rostedt@goodmis.org> (by way of Steven Rostedt
<rostedt@goodmis.org>) wrote:

> +++ b/kernel/trace/ftrace.c
> @@ -3654,6 +3654,31 @@ static void add_trampoline_func(struct seq_file *m, struct ftrace_ops *ops,
>  		seq_printf(m, " ->%pS", ptr);
>  }
>  
> +#ifdef FTRACE_MCOUNT_MAX_OFFSET
> +static int print_rec(struct seq_file *m, unsigned long ip)
> +{
> +	unsigned long offset;
> +	char str[KSYM_SYMBOL_LEN];
> +	char *modname;
> +	const char *ret;
> +
> +	ret = kallsyms_lookup(ip, NULL, &offset, &modname, str);
> +	if (!ret || offset > FTRACE_MCOUNT_MAX_OFFSET)
> +		return -1;

Unfortunately, I can't just skip printing these functions. The reason is
because it breaks trace-cmd (libtracefs specifically). As trace-cmd can
filter with full regular expressions (regex(3)), and does so by searching
the available_filter_functions. It collects an index of functions to
enabled, then passes that into set_ftrace_filter.

As a speed up, set_ftrace_filter allows you to pass an index, defined by the
line in available_filter_functions, into it that uses array indexing into
the ftrace table to enable/disable functions for tracing. By skipping
entries, it breaks the indexing, because the index is a 1 to 1 paring of
the lines of available_filter_functions.

To solve this, instead of printing nothing, I have this:

	ret = kallsyms_lookup(ip, NULL, &offset, &modname, str);
	/* Weak functions can cause invalid addresses */
	if (!ret || offset > FTRACE_MCOUNT_MAX_OFFSET) {
		snprintf(str, KSYM_SYMBOL_LEN, "%s_%ld",
			 FTRACE_INVALID_FUNCTION, offset);
	}

Where:

#define FTRACE_INVALID_FUNCTION		"__ftrace_invalid_address__"

When doing this, the available_filter_functions file has 546 invalid
entries. 14 of which are for the kvm module. Probably to deal with the
differences between Intel and AMD.

When a function is read as invalid, the rec->flags get set as DISABLED,
which will keep it from being enabled in the future.

Of course, one can just enter in numbers without reading any of the files,
and that will allow them to be set. It won't do anything bad, it would just
act like it does today.

Does anyone have any issues with this approach (with
__ftrace_invalid_address__%d inserted into available_filter_functions)?


-- Steve


> +
> +	seq_puts(m, str);
> +	if (modname)
> +		seq_printf(m, " [%s]", modname);
> +	return 0;
> +}
> +#else
> +static int print_rec(struct seq_file *m, unsigned long ip)
> +{
> +	seq_printf(m, "%ps", (void *)ip);
> +	return 0;
> +}
> +#endif
> +