[PATCH bpf-next v2 0/3] Fixes for Exceptions

[PATCH bpf-next v2 0/3] Fixes for Exceptions

[Kumar Kartikeya Dwivedi]

Set of fixes for bugs reported for the exceptions series.

Changelog:
----------
v1 -> v2
v1: https://lore.kernel.org/bpf/20230918143914.292526-1-memxor@gmail.com

 * Resend with trimmed Cc due to patchwork problem (Alexei)

Kumar Kartikeya Dwivedi (3):
  selftests/bpf: Print log buffer for exceptions test only on failure
  bpf: Fix bpf_throw warning on 32-bit arch
  bpf: Disable exceptions when CONFIG_UNWINDER_FRAME_POINTER=y

 arch/x86/net/bpf_jit_comp.c                         | 9 ++++-----
 kernel/bpf/helpers.c                                | 2 +-
 tools/testing/selftests/bpf/prog_tests/exceptions.c | 5 +++--
 3 files changed, 8 insertions(+), 8 deletions(-)


base-commit: ec6f1b4db95b7eedb3fe85f4f14e08fa0e9281c3
-- 
2.41.0

[PATCH bpf-next v2 1/3] selftests/bpf: Print log buffer for exceptions test only on failure

[Kumar Kartikeya Dwivedi]

Alexei reported seeing log messages for some test cases even though we
just wanted to match the error string from the verifier. Move the
printing of the log buffer to a guarded condition so that we only print
it when we fail to match on the expected string in the log buffer,
preventing unneeded output when running the test.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Fixes: d2a93715bfb0 ("selftests/bpf: Add tests for BPF exceptions")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/exceptions.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions.c b/tools/testing/selftests/bpf/prog_tests/exceptions.c
index 5663e427dc00..516f4a13013c 100644
--- a/tools/testing/selftests/bpf/prog_tests/exceptions.c
+++ b/tools/testing/selftests/bpf/prog_tests/exceptions.c
@@ -103,9 +103,10 @@ static void test_exceptions_success(void)
 			goto done;						  \
 		}								  \
 		if (load_ret != 0) {						  \
-			printf("%s\n", log_buf);				  \
-			if (!ASSERT_OK_PTR(strstr(log_buf, msg), "strstr"))	  \
+			if (!ASSERT_OK_PTR(strstr(log_buf, msg), "strstr")) {	  \
+				printf("%s\n", log_buf);			  \
 				goto done;					  \
+			}							  \
 		}								  \
 		if (!load_ret && attach_err) {					  \
 			if (!ASSERT_ERR_PTR(link = bpf_program__attach(prog), "attach err")) \
-- 
2.41.0

[PATCH bpf-next v2 2/3] bpf: Fix bpf_throw warning on 32-bit arch

[Kumar Kartikeya Dwivedi]

On 32-bit architectures, the pointer width is 32-bit, while we try to
cast from a u64 down to it, the compiler complains on mismatch in
integer size. Fix this by first casting to long which should match
the pointer width on targets supported by Linux.

Fixes: ec5290a178b7 ("bpf: Prevent KASAN false positive with bpf_throw")
Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 7ff2a42f1996..dd1c69ee3375 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2488,7 +2488,7 @@ __bpf_kfunc void bpf_throw(u64 cookie)
 	 * deeper stack depths than ctx.sp as we do not return from bpf_throw,
 	 * which skips compiler generated instrumentation to do the same.
 	 */
-	kasan_unpoison_task_stack_below((void *)ctx.sp);
+	kasan_unpoison_task_stack_below((void *)(long)ctx.sp);
 	ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp);
 	WARN(1, "A call to BPF exception callback should never return\n");
 }
-- 
2.41.0

Re: [PATCH bpf-next v2 2/3] bpf: Fix bpf_throw warning on 32-bit arch

[Matthieu Baerts]

Hi Kumar,

On 18/09/2023 17:52, Kumar Kartikeya Dwivedi wrote:
> On 32-bit architectures, the pointer width is 32-bit, while we try to
> cast from a u64 down to it, the compiler complains on mismatch in
> integer size. Fix this by first casting to long which should match
> the pointer width on targets supported by Linux.

Thank you for the patch, it fixes the issue on our side!

(Not sure you need a tested by tag but just in case: )

Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>

> Fixes: ec5290a178b7 ("bpf: Prevent KASAN false positive with bpf_throw")
> Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  kernel/bpf/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 7ff2a42f1996..dd1c69ee3375 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2488,7 +2488,7 @@ __bpf_kfunc void bpf_throw(u64 cookie)
>  	 * deeper stack depths than ctx.sp as we do not return from bpf_throw,
>  	 * which skips compiler generated instrumentation to do the same.
>  	 */
> -	kasan_unpoison_task_stack_below((void *)ctx.sp);
> +	kasan_unpoison_task_stack_below((void *)(long)ctx.sp);
I never know what's the recommended way to fix such issues: casting it
to 'long' or 'unsigned long'? But it looks like both are used in the
kernel and 'long' is more often used than the other one so all good I
suppose.

>  	ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp);
>  	WARN(1, "A call to BPF exception callback should never return\n");
>  }

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH bpf-next v2 2/3] bpf: Fix bpf_throw warning on 32-bit arch

[Kui-Feng Lee]

On 9/18/23 10:09, Matthieu Baerts wrote:
> Hi Kumar,
> 
> On 18/09/2023 17:52, Kumar Kartikeya Dwivedi wrote:
>> On 32-bit architectures, the pointer width is 32-bit, while we try to
>> cast from a u64 down to it, the compiler complains on mismatch in
>> integer size. Fix this by first casting to long which should match
>> the pointer width on targets supported by Linux.
> 
> Thank you for the patch, it fixes the issue on our side!
> 
> (Not sure you need a tested by tag but just in case: )
> 
> Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
>> Fixes: ec5290a178b7 ("bpf: Prevent KASAN false positive with bpf_throw")
>> Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> ---
>>   kernel/bpf/helpers.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 7ff2a42f1996..dd1c69ee3375 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -2488,7 +2488,7 @@ __bpf_kfunc void bpf_throw(u64 cookie)
>>   	 * deeper stack depths than ctx.sp as we do not return from bpf_throw,
>>   	 * which skips compiler generated instrumentation to do the same.
>>   	 */
>> -	kasan_unpoison_task_stack_below((void *)ctx.sp);
>> +	kasan_unpoison_task_stack_below((void *)(long)ctx.sp);
> I never know what's the recommended way to fix such issues: casting it
> to 'long' or 'unsigned long'? But it looks like both are used in the
> kernel and 'long' is more often used than the other one so all good I
> suppose.

Shouldn't we have a macro to do this kind of casting if there is not?
Without any comment, it is difficult to know why this extra casting
is here.

> 
>>   	ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp);
>>   	WARN(1, "A call to BPF exception callback should never return\n");
>>   }
> 
> Cheers,
> Matt

[PATCH bpf-next v2 3/3] bpf: Disable exceptions when CONFIG_UNWINDER_FRAME_POINTER=y

[Kumar Kartikeya Dwivedi]

The build with CONFIG_UNWINDER_FRAME_POINTER=y is broken for
current exceptions feature as it assumes ORC unwinder specific fields in
the unwind_state. Disable exceptions when frame_pointer unwinder is
enabled for now.

Fixes: fd5d27b70188 ("arch/x86: Implement arch_bpf_stack_walk")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 arch/x86/net/bpf_jit_comp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 84005f2114e0..8c10d9abc239 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3003,16 +3003,15 @@ void bpf_jit_free(struct bpf_prog *prog)
 bool bpf_jit_supports_exceptions(void)
 {
 	/* We unwind through both kernel frames (starting from within bpf_throw
-	 * call) and BPF frames. Therefore we require one of ORC or FP unwinder
-	 * to be enabled to walk kernel frames and reach BPF frames in the stack
-	 * trace.
+	 * call) and BPF frames. Therefore we require ORC unwinder to be enabled
+	 * to walk kernel frames and reach BPF frames in the stack trace.
 	 */
-	return IS_ENABLED(CONFIG_UNWINDER_ORC) || IS_ENABLED(CONFIG_UNWINDER_FRAME_POINTER);
+	return IS_ENABLED(CONFIG_UNWINDER_ORC);
 }
 
 void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp), void *cookie)
 {
-#if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER)
+#if defined(CONFIG_UNWINDER_ORC)
 	struct unwind_state state;
 	unsigned long addr;
 
-- 
2.41.0

Re: [PATCH bpf-next v2 3/3] bpf: Disable exceptions when CONFIG_UNWINDER_FRAME_POINTER=y

[Eric Dumazet]

On Mon, Sep 18, 2023 at 5:52 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> The build with CONFIG_UNWINDER_FRAME_POINTER=y is broken for
> current exceptions feature as it assumes ORC unwinder specific fields in
> the unwind_state. Disable exceptions when frame_pointer unwinder is
> enabled for now.
>
> Fixes: fd5d27b70188 ("arch/x86: Implement arch_bpf_stack_walk")
> Reported-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

SGTM, feel free adding

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH bpf-next v2 0/3] Fixes for Exceptions

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 18 Sep 2023 17:52:30 +0200 you wrote:
> Set of fixes for bugs reported for the exceptions series.
> 
> Changelog:
> ----------
> v1 -> v2
> v1: https://lore.kernel.org/bpf/20230918143914.292526-1-memxor@gmail.com
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/3] selftests/bpf: Print log buffer for exceptions test only on failure
    https://git.kernel.org/bpf/bpf-next/c/c425a3501f24
  - [bpf-next,v2,2/3] bpf: Fix bpf_throw warning on 32-bit arch
    https://git.kernel.org/bpf/bpf-next/c/00b7e8f4c02d
  - [bpf-next,v2,3/3] bpf: Disable exceptions when CONFIG_UNWINDER_FRAME_POINTER=y
    https://git.kernel.org/bpf/bpf-next/c/43c6e890472e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html