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

[Zheng Yejian]

On 2024/5/3 05:07, Steven Rostedt wrote:

On Wed, 17 Apr 2024 11:28:30 +0800
Zheng Yejian <zhengyeji...@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.

I'll do it in v4.

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?

Yes, the only-one-user function looks ugly.
At first thought that ftrace_location_range() needs to a lock, I just do like 
that,
no specital reason.

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;
+}