diff mbox series

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

Message ID 20240401191621.758056-1-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] rethook: Remove warning messages printed for finding return address of a frame. | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 945 this patch: 945
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: mathieu.desnoyers@efficios.com rostedt@goodmis.org
netdev/build_clang success Errors and warnings before: 955 this patch: 955
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 956 this patch: 956
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests

Commit Message

Kui-Feng Lee April 1, 2024, 7:16 p.m. UTC 
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(-)

Comments

Andrii Nakryiko April 2, 2024, 4:58 p.m. UTC | #1 
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
>
>
Daniel Borkmann April 3, 2024, 2:36 p.m. UTC | #2 
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
John Fastabend April 3, 2024, 10:15 p.m. UTC | #3 
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>
Masami Hiramatsu (Google) April 8, 2024, 1:13 a.m. UTC | #4 
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
Kui-Feng Lee April 8, 2024, 5:16 p.m. UTC | #5 
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
> 
>
diff mbox series

Patch

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 {