[PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[Kui-Feng Lee]

rethook_find_ret_addr() prints a warning message and returns 0 when the
target task is running and not the "current" task to prevent returning an
incorrect return address. However, this check is incomplete as the target
task can still transition to the running state when finding the return
address, although it is safe with RCU.

The issue we encounter is that the kernel frequently prints warning
messages when BPF profiling programs call to bpf_get_task_stack() on
running tasks.

The callers should be aware and willing to take the risk of receiving an
incorrect return address from a task that is currently running other than
the "current" one. A warning is not needed here as the callers are intent
on it.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/trace/rethook.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index fa03094e9e69..4297a132a7ae 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
 	if (WARN_ON_ONCE(!cur))
 		return 0;
 
-	if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
+	if (tsk != current && task_is_running(tsk))
 		return 0;
 
 	do {
-- 
2.34.1

Re: [PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[Andrii Nakryiko]

On Mon, Apr 1, 2024 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> rethook_find_ret_addr() prints a warning message and returns 0 when the
> target task is running and not the "current" task to prevent returning an
> incorrect return address. However, this check is incomplete as the target
> task can still transition to the running state when finding the return
> address, although it is safe with RCU.
>
> The issue we encounter is that the kernel frequently prints warning
> messages when BPF profiling programs call to bpf_get_task_stack() on
> running tasks.
>
> The callers should be aware and willing to take the risk of receiving an
> incorrect return address from a task that is currently running other than
> the "current" one. A warning is not needed here as the callers are intent
> on it.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  kernel/trace/rethook.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index fa03094e9e69..4297a132a7ae 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
>         if (WARN_ON_ONCE(!cur))
>                 return 0;
>
> -       if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
> +       if (tsk != current && task_is_running(tsk))
>                 return 0;
>

This should probably go through Masami's tree, but the change makes
sense to me, given this is an expected condition.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>         do {
> --
> 2.34.1
>
>

Re: [PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[Daniel Borkmann]

On 4/2/24 6:58 PM, Andrii Nakryiko wrote:
> On Mon, Apr 1, 2024 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> rethook_find_ret_addr() prints a warning message and returns 0 when the
>> target task is running and not the "current" task to prevent returning an
>> incorrect return address. However, this check is incomplete as the target
>> task can still transition to the running state when finding the return
>> address, although it is safe with RCU.
>>
>> The issue we encounter is that the kernel frequently prints warning
>> messages when BPF profiling programs call to bpf_get_task_stack() on
>> running tasks.
>>
>> The callers should be aware and willing to take the risk of receiving an
>> incorrect return address from a task that is currently running other than
>> the "current" one. A warning is not needed here as the callers are intent
>> on it.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/trace/rethook.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
>> index fa03094e9e69..4297a132a7ae 100644
>> --- a/kernel/trace/rethook.c
>> +++ b/kernel/trace/rethook.c
>> @@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
>>          if (WARN_ON_ONCE(!cur))
>>                  return 0;
>>
>> -       if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
>> +       if (tsk != current && task_is_running(tsk))
>>                  return 0;
>>
> 
> This should probably go through Masami's tree, but the change makes
> sense to me, given this is an expected condition.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Masami, I assume you'll pick this up?

Thanks,
Daniel

RE: [PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[John Fastabend]

Kui-Feng Lee wrote:
> rethook_find_ret_addr() prints a warning message and returns 0 when the
> target task is running and not the "current" task to prevent returning an
> incorrect return address. However, this check is incomplete as the target
> task can still transition to the running state when finding the return
> address, although it is safe with RCU.
> 
> The issue we encounter is that the kernel frequently prints warning
> messages when BPF profiling programs call to bpf_get_task_stack() on
> running tasks.
> 
> The callers should be aware and willing to take the risk of receiving an
> incorrect return address from a task that is currently running other than
> the "current" one. A warning is not needed here as the callers are intent
> on it.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  kernel/trace/rethook.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index fa03094e9e69..4297a132a7ae 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
>  	if (WARN_ON_ONCE(!cur))
>  		return 0;
>  
> -	if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
> +	if (tsk != current && task_is_running(tsk))
>  		return 0;
>  
>  	do {
> -- 
> 2.34.1
> 
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[Masami Hiramatsu]

On Wed, 3 Apr 2024 16:36:25 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 4/2/24 6:58 PM, Andrii Nakryiko wrote:
> > On Mon, Apr 1, 2024 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
> >>
> >> rethook_find_ret_addr() prints a warning message and returns 0 when the
> >> target task is running and not the "current" task to prevent returning an
> >> incorrect return address. However, this check is incomplete as the target
> >> task can still transition to the running state when finding the return
> >> address, although it is safe with RCU.

Could you tell me more about this last part? This change just remove
WARN_ON_ONCE() which warns that the user tries to unwind stack of a running
task. This means the task can change the stack in parallel if the task is
running on other CPU.
Does the BPF stop the task? or do you have any RCU magic to copy the stack?

> >>
> >> The issue we encounter is that the kernel frequently prints warning
> >> messages when BPF profiling programs call to bpf_get_task_stack() on
> >> running tasks.

Hmm, WARN_ON_ONCE should print it once, not frequently.

> >>
> >> The callers should be aware and willing to take the risk of receiving an
> >> incorrect return address from a task that is currently running other than
> >> the "current" one. A warning is not needed here as the callers are intent
> >> on it.
> >>
> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> >> ---
> >>   kernel/trace/rethook.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> >> index fa03094e9e69..4297a132a7ae 100644
> >> --- a/kernel/trace/rethook.c
> >> +++ b/kernel/trace/rethook.c
> >> @@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
> >>          if (WARN_ON_ONCE(!cur))
> >>                  return 0;
> >>
> >> -       if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
> >> +       if (tsk != current && task_is_running(tsk))
> >>                  return 0;
> >>
> > 
> > This should probably go through Masami's tree, but the change makes
> > sense to me, given this is an expected condition.
> > 
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> Masami, I assume you'll pick this up?

OK, anyway it will just return 0 if this situation happens, and caller will
get the trampoline address instead of correct return address in this case.
I think it does not do any unsafe things. So I agree removing it.
But I think the explanation is a bit confusing.

Thank you,

> 
> Thanks,
> Daniel


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH bpf-next] rethook: Remove warning messages printed for finding return address of a frame.

[Kui-Feng Lee]

On 4/7/24 18:13, Masami Hiramatsu (Google) wrote:
> On Wed, 3 Apr 2024 16:36:25 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
>> On 4/2/24 6:58 PM, Andrii Nakryiko wrote:
>>> On Mon, Apr 1, 2024 at 12:16 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>>>
>>>> rethook_find_ret_addr() prints a warning message and returns 0 when the
>>>> target task is running and not the "current" task to prevent returning an
>>>> incorrect return address. However, this check is incomplete as the target
>>>> task can still transition to the running state when finding the return
>>>> address, although it is safe with RCU.
> 
> Could you tell me more about this last part? This change just remove
> WARN_ON_ONCE() which warns that the user tries to unwind stack of a running
> task. This means the task can change the stack in parallel if the task is
> running on other CPU.
> Does the BPF stop the task? or do you have any RCU magic to copy the stack?


No, the BPF doesn't stop the task or copy the stack. The last part tries
to explain that this function can still return an incorrect address even
with this check. And calling this function on a target task that is not
"current" is safe.  Since you think it is confusing. I will remove this
part.

> 
>>>>
>>>> The issue we encounter is that the kernel frequently prints warning
>>>> messages when BPF profiling programs call to bpf_get_task_stack() on
>>>> running tasks.
> 
> Hmm, WARN_ON_ONCE should print it once, not frequently.

You are right! I should rephrase it. In a firm with a large number of 
hosts, this warning message become a noise.

> 
>>>>
>>>> The callers should be aware and willing to take the risk of receiving an
>>>> incorrect return address from a task that is currently running other than
>>>> the "current" one. A warning is not needed here as the callers are intent
>>>> on it.
>>>>
>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>> ---
>>>>    kernel/trace/rethook.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
>>>> index fa03094e9e69..4297a132a7ae 100644
>>>> --- a/kernel/trace/rethook.c
>>>> +++ b/kernel/trace/rethook.c
>>>> @@ -248,7 +248,7 @@ unsigned long rethook_find_ret_addr(struct task_struct *tsk, unsigned long frame
>>>>           if (WARN_ON_ONCE(!cur))
>>>>                   return 0;
>>>>
>>>> -       if (WARN_ON_ONCE(tsk != current && task_is_running(tsk)))
>>>> +       if (tsk != current && task_is_running(tsk))
>>>>                   return 0;
>>>>
>>>
>>> This should probably go through Masami's tree, but the change makes
>>> sense to me, given this is an expected condition.
>>>
>>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>>
>> Masami, I assume you'll pick this up?
> 
> OK, anyway it will just return 0 if this situation happens, and caller will
> get the trampoline address instead of correct return address in this case.
> I think it does not do any unsafe things. So I agree removing it.
> But I think the explanation is a bit confusing.
> 
> Thank you,
> 
>>
>> Thanks,
>> Daniel
> 
>