Linus,

Fix for function graph tracer in v6.13:

- Add needed READ_ONCE() around access to the fgraph array element

  The updates to the fgraph array can happen when callbacks are registered
  and deregistered. The __ftrace_return_to_handler() can handle reading
  either the old value or the new value. But once it reads that value
  it must stay consistent otherwise the check that looks to see if the
  value is a stub may show false, but if the compiler decides to re-read
  after that check, it can be true which can cause the code to crash
  later on.


Please pull the latest ftrace-v6.13-rc5 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
ftrace-v6.13-rc5

Tag SHA1: 0ba23030ff298e46f7d435ee1e11040d00239871
Head SHA1: e99b3d0cb6cd0b051fea113bb9a4088d595c4693


Zilin Guan (1):
      fgraph: Add READ_ONCE() when accessing fgraph_array[]

----
 kernel/trace/fgraph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---------------------------
commit e99b3d0cb6cd0b051fea113bb9a4088d595c4693
Author: Zilin Guan <zilin@seu.edu.cn>
Date:   Tue Dec 31 11:37:31 2024 +0000

    fgraph: Add READ_ONCE() when accessing fgraph_array[]
    
    In __ftrace_return_to_handler(), a loop iterates over the fgraph_array[]
    elements, which are fgraph_ops. The loop checks if an element is a
    fgraph_stub to prevent using a fgraph_stub afterward.
    
    However, if the compiler reloads fgraph_array[] after this check, it might
    race with an update to fgraph_array[] that introduces a fgraph_stub. This
    could result in the stub being processed, but the stub contains a null
    "func_hash" field, leading to a NULL pointer dereference.
    
    To ensure that the gops compared against the fgraph_stub matches the gops
    processed later, add a READ_ONCE(). A similar patch appears in commit
    63a8dfb ("function_graph: Add READ_ONCE() when accessing fgraph_array[]").
    
    Cc:stable@vger.kernel.org
    Fixes: 37238abe3cb47 ("ftrace/function_graph: Pass fgraph_ops to function graph callbacks")
    Link: https://lore.kernel.org/20241231113731.277668-1-zilin@seu.edu.cn
    Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
    Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index ddedcb50917f..30e3ddc8a8a8 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -833,7 +833,7 @@ static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs
 #endif
 	{
 		for_each_set_bit(i, &bitmap, sizeof(bitmap) * BITS_PER_BYTE) {
-			struct fgraph_ops *gops = fgraph_array[i];
+			struct fgraph_ops *gops = READ_ONCE(fgraph_array[i]);
 
 			if (gops == &fgraph_stub)
 				continue;

On Thu, 2 Jan 2025 10:20:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Linus,
> 
> Fix for function graph tracer in v6.13:

Linus,

If you want, you can hold off on pulling this. There's another unrelated
fix to ftrace that I need to add and test. Then I can send a pull request
for both fixes instead of two individual ones.

-- Steve


> 
> - Add needed READ_ONCE() around access to the fgraph array element
> 
>   The updates to the fgraph array can happen when callbacks are registered
>   and deregistered. The __ftrace_return_to_handler() can handle reading
>   either the old value or the new value. But once it reads that value
>   it must stay consistent otherwise the check that looks to see if the
>   value is a stub may show false, but if the compiler decides to re-read
>   after that check, it can be true which can cause the code to crash
>   later on.
> 
> 
> Please pull the latest ftrace-v6.13-rc5 tree, which can be found at:
> 
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
> ftrace-v6.13-rc5
> 
> Tag SHA1: 0ba23030ff298e46f7d435ee1e11040d00239871
> Head SHA1: e99b3d0cb6cd0b051fea113bb9a4088d595c4693
> 
> 
> Zilin Guan (1):
>       fgraph: Add READ_ONCE() when accessing fgraph_array[]
> 
> ----
>  kernel/trace/fgraph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> ---------------------------
> commit e99b3d0cb6cd0b051fea113bb9a4088d595c4693
> Author: Zilin Guan <zilin@seu.edu.cn>
> Date:   Tue Dec 31 11:37:31 2024 +0000
> 
>     fgraph: Add READ_ONCE() when accessing fgraph_array[]
>     
>     In __ftrace_return_to_handler(), a loop iterates over the fgraph_array[]
>     elements, which are fgraph_ops. The loop checks if an element is a
>     fgraph_stub to prevent using a fgraph_stub afterward.
>     
>     However, if the compiler reloads fgraph_array[] after this check, it might
>     race with an update to fgraph_array[] that introduces a fgraph_stub. This
>     could result in the stub being processed, but the stub contains a null
>     "func_hash" field, leading to a NULL pointer dereference.
>     
>     To ensure that the gops compared against the fgraph_stub matches the gops
>     processed later, add a READ_ONCE(). A similar patch appears in commit
>     63a8dfb ("function_graph: Add READ_ONCE() when accessing fgraph_array[]").
>     
>     Cc:stable@vger.kernel.org
>     Fixes: 37238abe3cb47 ("ftrace/function_graph: Pass fgraph_ops to function graph callbacks")
>     Link: https://lore.kernel.org/20241231113731.277668-1-zilin@seu.edu.cn
>     Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
>     Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> 
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index ddedcb50917f..30e3ddc8a8a8 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -833,7 +833,7 @@ static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs
>  #endif
>  	{
>  		for_each_set_bit(i, &bitmap, sizeof(bitmap) * BITS_PER_BYTE) {
> -			struct fgraph_ops *gops = fgraph_array[i];
> +			struct fgraph_ops *gops = READ_ONCE(fgraph_array[i]);
>  
>  			if (gops == &fgraph_stub)
>  				continue;

On Thu, 2 Jan 2025 at 07:33, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> If you want, you can hold off on pulling this. There's another unrelated
> fix to ftrace that I need to add and test. Then I can send a pull request
> for both fixes instead of two individual ones.

Ok, will wait.

           Linus