[PATCH 0/2] bpf: Recursion detection related fixes.

[PATCH 0/2] bpf: Recursion detection related fixes.

[Sebastian Andrzej Siewior]

Hi,

the two things popped up during review. Compile tested only.

Sebastian

[PATCH 1/2] bpf: Invoke __bpf_prog_exit_sleepable_recur() on recursion in kern_sys_bpf().

[Sebastian Andrzej Siewior]

If __bpf_prog_enter_sleepable_recur() detects recursion then it returns
0 without undoing rcu_read_lock_trace(), migrate_disable() or
decrementing the recursion counter. This is fine in the JIT case because
the JIT code will jump in the 0 case to the end and invoke the matching
exit trampoline (__bpf_prog_exit_sleepable_recur()).

This is not the case in kern_sys_bpf() which returns directly to the
caller with an error code.

Add __bpf_prog_exit_sleepable_recur() as clean up in the recursion case.

Fixes: b1d18a7574d0d ("bpf: Extend sys_bpf commands for bpf_syscall programs.")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/bpf/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a2aef900519c2..c925c270ed8b4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5307,6 +5307,7 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size)
 		run_ctx.saved_run_ctx = NULL;
 		if (!__bpf_prog_enter_sleepable_recur(prog, &run_ctx)) {
 			/* recursion detected */
+			__bpf_prog_exit_sleepable_recur(prog, 0, &run_ctx);
 			bpf_prog_put(prog);
 			return -EBUSY;
 		}
-- 
2.40.1

Re: [PATCH 1/2] bpf: Invoke __bpf_prog_exit_sleepable_recur() on recursion in kern_sys_bpf().

[Jiri Olsa]

On Wed, Aug 30, 2023 at 10:04:04AM +0200, Sebastian Andrzej Siewior wrote:
> If __bpf_prog_enter_sleepable_recur() detects recursion then it returns
> 0 without undoing rcu_read_lock_trace(), migrate_disable() or
> decrementing the recursion counter. This is fine in the JIT case because
> the JIT code will jump in the 0 case to the end and invoke the matching
> exit trampoline (__bpf_prog_exit_sleepable_recur()).
> 
> This is not the case in kern_sys_bpf() which returns directly to the
> caller with an error code.
> 
> Add __bpf_prog_exit_sleepable_recur() as clean up in the recursion case.
> 
> Fixes: b1d18a7574d0d ("bpf: Extend sys_bpf commands for bpf_syscall programs.")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  kernel/bpf/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a2aef900519c2..c925c270ed8b4 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5307,6 +5307,7 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size)
>  		run_ctx.saved_run_ctx = NULL;
>  		if (!__bpf_prog_enter_sleepable_recur(prog, &run_ctx)) {
>  			/* recursion detected */
> +			__bpf_prog_exit_sleepable_recur(prog, 0, &run_ctx);
>  			bpf_prog_put(prog);
>  			return -EBUSY;
>  		}
> -- 
> 2.40.1
> 

[PATCH 2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.

[Sebastian Andrzej Siewior]

__bpf_prog_enter() assigns bpf_tramp_run_ctx::saved_run_ctx before
performing the recursion check which means in case of a recursion
__bpf_prog_exit() uses the previously set
bpf_tramp_run_ctx::saved_run_ctx value.

__bpf_prog_enter_sleepable() assigns bpf_tramp_run_ctx::saved_run_ctx
after the recursion check which means in case of a recursion
__bpf_prog_exit_sleepable() uses an uninitialized value.
This does not look right. If I read the entry trampoline code right,
then bpf_tramp_run_ctx isn't initialized upfront.

Align __bpf_prog_enter_sleepable() with __bpf_prog_enter() and set
bpf_tramp_run_ctx::saved_run_ctx before the recursion check is made.
Remove the assignment of saved_run_ctx in kern_sys_bpf() since it
happens a few cycles later.

Fixes: e384c7b7b46d0 ("bpf, x86: Create bpf_tramp_run_ctx on the caller thread's stack")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/bpf/syscall.c    | 1 -
 kernel/bpf/trampoline.c | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c925c270ed8b4..1480b6cf12f06 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5304,7 +5304,6 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size)
 		}
 
 		run_ctx.bpf_cookie = 0;
-		run_ctx.saved_run_ctx = NULL;
 		if (!__bpf_prog_enter_sleepable_recur(prog, &run_ctx)) {
 			/* recursion detected */
 			__bpf_prog_exit_sleepable_recur(prog, 0, &run_ctx);
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 78acf28d48732..53ff50cac61ea 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -926,13 +926,12 @@ u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
 	migrate_disable();
 	might_fault();
 
+	run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
+
 	if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
 		bpf_prog_inc_misses_counter(prog);
 		return 0;
 	}
-
-	run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
-
 	return bpf_prog_start_time();
 }
 
-- 
2.40.1

Re: [PATCH 2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.

[Jiri Olsa]

On Wed, Aug 30, 2023 at 10:04:05AM +0200, Sebastian Andrzej Siewior wrote:
> __bpf_prog_enter() assigns bpf_tramp_run_ctx::saved_run_ctx before

I guess you meant __bpf_prog_enter_recur right?

> performing the recursion check which means in case of a recursion
> __bpf_prog_exit() uses the previously set
> bpf_tramp_run_ctx::saved_run_ctx value.
> 
> __bpf_prog_enter_sleepable() assigns bpf_tramp_run_ctx::saved_run_ctx

__bpf_prog_enter_sleepable_recur ?

> after the recursion check which means in case of a recursion
> __bpf_prog_exit_sleepable() uses an uninitialized value.
> This does not look right. If I read the entry trampoline code right,
> then bpf_tramp_run_ctx isn't initialized upfront.
> 
> Align __bpf_prog_enter_sleepable() with __bpf_prog_enter() and set

ditto

> bpf_tramp_run_ctx::saved_run_ctx before the recursion check is made.
> Remove the assignment of saved_run_ctx in kern_sys_bpf() since it
> happens a few cycles later.
> 
> Fixes: e384c7b7b46d0 ("bpf, x86: Create bpf_tramp_run_ctx on the caller thread's stack")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

makes sense to me.. I ran selftests and all passed
CI seems to fail due to unrelated issues that are just being fixed

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  kernel/bpf/syscall.c    | 1 -
>  kernel/bpf/trampoline.c | 5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c925c270ed8b4..1480b6cf12f06 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5304,7 +5304,6 @@ int kern_sys_bpf(int cmd, union bpf_attr *attr, unsigned int size)
>  		}
>  
>  		run_ctx.bpf_cookie = 0;
> -		run_ctx.saved_run_ctx = NULL;
>  		if (!__bpf_prog_enter_sleepable_recur(prog, &run_ctx)) {
>  			/* recursion detected */
>  			__bpf_prog_exit_sleepable_recur(prog, 0, &run_ctx);
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 78acf28d48732..53ff50cac61ea 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -926,13 +926,12 @@ u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
>  	migrate_disable();
>  	might_fault();
>  
> +	run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> +
>  	if (unlikely(this_cpu_inc_return(*(prog->active)) != 1)) {
>  		bpf_prog_inc_misses_counter(prog);
>  		return 0;
>  	}
> -
> -	run_ctx->saved_run_ctx = bpf_set_run_ctx(&run_ctx->run_ctx);
> -
>  	return bpf_prog_start_time();
>  }
>  
> -- 
> 2.40.1
> 

Re: [PATCH 2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.

[Sebastian Andrzej Siewior]

On 2023-09-01 16:13:04 [+0200], Jiri Olsa wrote:
> On Wed, Aug 30, 2023 at 10:04:05AM +0200, Sebastian Andrzej Siewior wrote:
> > __bpf_prog_enter() assigns bpf_tramp_run_ctx::saved_run_ctx before
> 
> I guess you meant __bpf_prog_enter_recur right?
> 
> > performing the recursion check which means in case of a recursion
> > __bpf_prog_exit() uses the previously set
> > bpf_tramp_run_ctx::saved_run_ctx value.
> > 
> > __bpf_prog_enter_sleepable() assigns bpf_tramp_run_ctx::saved_run_ctx
> 
> __bpf_prog_enter_sleepable_recur ?
> 
> > after the recursion check which means in case of a recursion
> > __bpf_prog_exit_sleepable() uses an uninitialized value.
> > This does not look right. If I read the entry trampoline code right,
> > then bpf_tramp_run_ctx isn't initialized upfront.
> > 
> > Align __bpf_prog_enter_sleepable() with __bpf_prog_enter() and set
> 
> ditto

Yes, in both cases. The ones I mentioned have no conditionals. Sorry.

> jirka

Sebastian

Re: [PATCH 2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.

[Daniel Borkmann]

On 9/1/23 4:19 PM, Sebastian Andrzej Siewior wrote:
> On 2023-09-01 16:13:04 [+0200], Jiri Olsa wrote:
>> On Wed, Aug 30, 2023 at 10:04:05AM +0200, Sebastian Andrzej Siewior wrote:
>>> __bpf_prog_enter() assigns bpf_tramp_run_ctx::saved_run_ctx before
>>
>> I guess you meant __bpf_prog_enter_recur right?
>>
>>> performing the recursion check which means in case of a recursion
>>> __bpf_prog_exit() uses the previously set
>>> bpf_tramp_run_ctx::saved_run_ctx value.
>>>
>>> __bpf_prog_enter_sleepable() assigns bpf_tramp_run_ctx::saved_run_ctx
>>
>> __bpf_prog_enter_sleepable_recur ?
>>
>>> after the recursion check which means in case of a recursion
>>> __bpf_prog_exit_sleepable() uses an uninitialized value.
>>> This does not look right. If I read the entry trampoline code right,
>>> then bpf_tramp_run_ctx isn't initialized upfront.
>>>
>>> Align __bpf_prog_enter_sleepable() with __bpf_prog_enter() and set
>>
>> ditto
> 
> Yes, in both cases. The ones I mentioned have no conditionals. Sorry.

Sebastian, I fixed this up and also the __bpf_prog_exit*() presumably should
have been the _recur flavor.

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=6764e767f4af1e35f87f3497e1182d945de37f93

Thanks,
Daniel

Re: [PATCH 2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.

[Sebastian Andrzej Siewior]

On 2023-09-06 10:52:36 [+0200], Daniel Borkmann wrote:
> Sebastian, I fixed this up and also the __bpf_prog_exit*() presumably should
> have been the _recur flavor.

I'm sorry, for not following up in time. Thank you.

> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=6764e767f4af1e35f87f3497e1182d945de37f93
> 
> Thanks,
> Daniel

Sebastian

Re: [PATCH 0/2] bpf: Recursion detection related fixes.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Wed, 30 Aug 2023 10:04:03 +0200 you wrote:
> Hi,
> 
> the two things popped up during review. Compile tested only.
> 
> Sebastian

Here is the summary with links:
  - [1/2] bpf: Invoke __bpf_prog_exit_sleepable_recur() on recursion in kern_sys_bpf().
    https://git.kernel.org/bpf/bpf/c/7645629f7dc8
  - [2/2] bpf: Assign bpf_tramp_run_ctx::saved_run_ctx before recursion check.
    https://git.kernel.org/bpf/bpf/c/6764e767f4af

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