Re: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace_location()

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 17 Apr 2024 11:28:30 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da1710499698..e05d3e3dc06a 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>  }
>  
>  /**
> - * ftrace_location_range - return the first address of a traced location
> + * ftrace_location_range_rcu - return the first address of a traced location

kerneldoc comments are for external functions. You need to move this down
to ftrace_location_range() as here you are commenting a local static function.

But I have to ask, why did you create this static function anyway? There's
only one user of it (the ftrace_location_range()). Why didn't you just
simply add the rcu locking there?

unsigned long ftrace_location_range(unsigned long start, unsigned long end)
{
	struct dyn_ftrace *rec;
	unsigned long ip = 0;

	rcu_read_lock();
	rec = lookup_rec(start, end);
	if (rec)
		ip = rec->ip;
	rcu_read_unlock();

	return ip;
}

-- Steve


>   *	if it touches the given ip range
>   * @start: start of range to search.
>   * @end: end of range to search (inclusive). @end points to the last byte
> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>   * that is either a NOP or call to the function tracer. It checks the ftrace
>   * internal tables to determine if the address belongs or not.
>   */
> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
>  {
>  	struct dyn_ftrace *rec;
>  
> @@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +{
> +	unsigned long loc;
> +
> +	rcu_read_lock();
> +	loc = ftrace_location_range_rcu(start, end);
> +	rcu_read_unlock();
> +	return loc;
> +}