[PATCH bpf-next v1 0/2] Enable static subprog calls in spin lock critical sections

[PATCH bpf-next v1 0/2] Enable static subprog calls in spin lock critical sections

[Kumar Kartikeya Dwivedi]

This set allows a BPF program to make a call to a static subprog within
a bpf_spin_lock critical section. This problem has been hit in sched-ext
and ghOSt [0] as well, and is mostly an annoyance which is worked around
by inling the static subprog into the critical section.

In case of sched-ext, there are a lot of other helper/kfunc calls that
need to be allow listed for the support to be complete, but a separate
follow up will deal with that.

Unlike static subprogs, global subprogs cannot be allowed yet as the
verifier will not explore their body when encountering a call
instruction for them. Therefore, we would need an alternative approach
(some sort of function summarization to ensure a lock is never taken
from a global subprog and all its callees).

 [0]: https://lore.kernel.org/bpf/bd173bf2-dea6-3e0e-4176-4a9256a9a056@google.com

Kumar Kartikeya Dwivedi (2):
  bpf: Allow calling static subprogs while holding a bpf_spin_lock
  selftests/bpf: Add test for static subprog call in lock cs

 kernel/bpf/verifier.c                         | 10 ++-
 .../selftests/bpf/prog_tests/spin_lock.c      |  2 +
 .../selftests/bpf/progs/test_spin_lock.c      | 65 +++++++++++++++++++
 .../selftests/bpf/progs/test_spin_lock_fail.c | 44 +++++++++++++
 .../selftests/bpf/progs/verifier_spin_lock.c  |  2 +-
 5 files changed, 119 insertions(+), 4 deletions(-)


base-commit: 2a79690eae953daaac232f93e6c5ac47ac539f2d
-- 
2.40.1

[PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Kumar Kartikeya Dwivedi]

Currently, calling any helpers, kfuncs, or subprogs except the graph
data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
is not allowed. One of the original motivations of this decision was to
force the BPF programmer's hand into keeping the bpf_spin_lock critical
section small, and to ensure the execution time of the program does not
increase due to lock waiting times. In addition to this, some of the
helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.

However, when it comes to subprog calls, atleast for static subprogs,
the verifier is able to explore their instructions during verification.
Therefore, it is similar in effect to having the same code inlined into
the critical section. Hence, not allowing static subprog calls in the
bpf_spin_lock critical section is mostly an annoyance that needs to be
worked around, without providing any tangible benefit.

Unlike static subprog calls, global subprog calls are not safe to permit
within the critical section, as the verifier does not explore them
during verification, therefore whether the same lock will be taken
again, or unlocked, cannot be ascertained.

Therefore, allow calling static subprogs within a bpf_spin_lock critical
section, and only reject it in case the subprog linkage is global.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c                                  | 10 +++++++---
 tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 64fa188d00ad..f858c959753b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	if (subprog_is_global(env, subprog)) {
 		const char *sub_name = subprog_name(env, subprog);
 
+		/* Only global subprogs cannot be called with a lock held. */
+		if (env->cur_state->active_lock.ptr) {
+			verbose(env, "function calls are not allowed while holding a lock\n");
+			return -EINVAL;
+		}
+
 		if (err) {
 			verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
 				subprog, sub_name);
@@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
 
 				if (env->cur_state->active_lock.ptr) {
 					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
-					    (insn->src_reg == BPF_PSEUDO_CALL) ||
 					    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
 					     (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
 						verbose(env, "function calls are not allowed while holding a lock\n");
@@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
 					return -EINVAL;
 				}
 process_bpf_exit_full:
-				if (env->cur_state->active_lock.ptr &&
-				    !in_rbtree_lock_required_cb(env)) {
+				if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
 					verbose(env, "bpf_spin_unlock is missing\n");
 					return -EINVAL;
 				}
diff --git a/tools/testing/selftests/bpf/progs/verifier_spin_lock.c b/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
index 9c1aa69650f8..fb316c080c84 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
@@ -330,7 +330,7 @@ l1_%=:	r7 = r0;					\
 
 SEC("cgroup/skb")
 __description("spin_lock: test10 lock in subprog without unlock")
-__failure __msg("unlock is missing")
+__success
 __failure_unpriv __msg_unpriv("")
 __naked void lock_in_subprog_without_unlock(void)
 {
-- 
2.40.1

Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Yonghong Song]

On 2/4/24 4:02 AM, Kumar Kartikeya Dwivedi wrote:
> Currently, calling any helpers, kfuncs, or subprogs except the graph
> data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
> is not allowed. One of the original motivations of this decision was to
> force the BPF programmer's hand into keeping the bpf_spin_lock critical
> section small, and to ensure the execution time of the program does not
> increase due to lock waiting times. In addition to this, some of the
> helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
>
> However, when it comes to subprog calls, atleast for static subprogs,
> the verifier is able to explore their instructions during verification.
> Therefore, it is similar in effect to having the same code inlined into
> the critical section. Hence, not allowing static subprog calls in the
> bpf_spin_lock critical section is mostly an annoyance that needs to be
> worked around, without providing any tangible benefit.
>
> Unlike static subprog calls, global subprog calls are not safe to permit
> within the critical section, as the verifier does not explore them
> during verification, therefore whether the same lock will be taken
> again, or unlocked, cannot be ascertained.
>
> Therefore, allow calling static subprogs within a bpf_spin_lock critical
> section, and only reject it in case the subprog linkage is global.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

SGTM with a small nit below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   kernel/bpf/verifier.c                                  | 10 +++++++---
>   tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
>   2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 64fa188d00ad..f858c959753b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>   	if (subprog_is_global(env, subprog)) {
>   		const char *sub_name = subprog_name(env, subprog);
>   
> +		/* Only global subprogs cannot be called with a lock held. */
> +		if (env->cur_state->active_lock.ptr) {
> +			verbose(env, "function calls are not allowed while holding a lock\n");

Maybe explicit to mention "global function calls are not allowed ..."?

> +			return -EINVAL;
> +		}
> +
>   		if (err) {
>   			verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
>   				subprog, sub_name);
> @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
>   
>   				if (env->cur_state->active_lock.ptr) {
>   					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> -					    (insn->src_reg == BPF_PSEUDO_CALL) ||
>   					    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>   					     (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
>   						verbose(env, "function calls are not allowed while holding a lock\n");
> @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
>   					return -EINVAL;
>   				}
>   process_bpf_exit_full:
> -				if (env->cur_state->active_lock.ptr &&
> -				    !in_rbtree_lock_required_cb(env)) {
> +				if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
>   					verbose(env, "bpf_spin_unlock is missing\n");
>   					return -EINVAL;
>   				}

[...]

Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Kumar Kartikeya Dwivedi]

On Sun, 4 Feb 2024 at 22:23, Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 2/4/24 4:02 AM, Kumar Kartikeya Dwivedi wrote:
> > Currently, calling any helpers, kfuncs, or subprogs except the graph
> > data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
> > is not allowed. One of the original motivations of this decision was to
> > force the BPF programmer's hand into keeping the bpf_spin_lock critical
> > section small, and to ensure the execution time of the program does not
> > increase due to lock waiting times. In addition to this, some of the
> > helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
> >
> > However, when it comes to subprog calls, atleast for static subprogs,
> > the verifier is able to explore their instructions during verification.
> > Therefore, it is similar in effect to having the same code inlined into
> > the critical section. Hence, not allowing static subprog calls in the
> > bpf_spin_lock critical section is mostly an annoyance that needs to be
> > worked around, without providing any tangible benefit.
> >
> > Unlike static subprog calls, global subprog calls are not safe to permit
> > within the critical section, as the verifier does not explore them
> > during verification, therefore whether the same lock will be taken
> > again, or unlocked, cannot be ascertained.
> >
> > Therefore, allow calling static subprogs within a bpf_spin_lock critical
> > section, and only reject it in case the subprog linkage is global.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> SGTM with a small nit below.
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
> > ---
> >   kernel/bpf/verifier.c                                  | 10 +++++++---
> >   tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
> >   2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 64fa188d00ad..f858c959753b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >       if (subprog_is_global(env, subprog)) {
> >               const char *sub_name = subprog_name(env, subprog);
> >
> > +             /* Only global subprogs cannot be called with a lock held. */
> > +             if (env->cur_state->active_lock.ptr) {
> > +                     verbose(env, "function calls are not allowed while holding a lock\n");
>
> Maybe explicit to mention "global function calls are not allowed ..."?
>

Ack, I made this change, and also extended the last part with "use a
static function instead" to make it more helpful.
Thanks for the review.

> > +                     return -EINVAL;
> > +             }
> > +
> >               if (err) {
> >                       verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
> >                               subprog, sub_name);
> > @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
> >
> >                               if (env->cur_state->active_lock.ptr) {
> >                                       if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> > -                                         (insn->src_reg == BPF_PSEUDO_CALL) ||
> >                                           (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> >                                            (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
> >                                               verbose(env, "function calls are not allowed while holding a lock\n");
> > @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
> >                                       return -EINVAL;
> >                               }
> >   process_bpf_exit_full:
> > -                             if (env->cur_state->active_lock.ptr &&
> > -                                 !in_rbtree_lock_required_cb(env)) {
> > +                             if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
> >                                       verbose(env, "bpf_spin_unlock is missing\n");
> >                                       return -EINVAL;
> >                               }
>
> [...]
>

Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[David Vernet]

[-- Attachment #1: Type: text/plain, Size: 4532 bytes --]

On Sun, Feb 04, 2024 at 12:02:05PM +0000, Kumar Kartikeya Dwivedi wrote:
> Currently, calling any helpers, kfuncs, or subprogs except the graph
> data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
> is not allowed. One of the original motivations of this decision was to
> force the BPF programmer's hand into keeping the bpf_spin_lock critical
> section small, and to ensure the execution time of the program does not
> increase due to lock waiting times. In addition to this, some of the
> helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
> 
> However, when it comes to subprog calls, atleast for static subprogs,
> the verifier is able to explore their instructions during verification.
> Therefore, it is similar in effect to having the same code inlined into
> the critical section. Hence, not allowing static subprog calls in the
> bpf_spin_lock critical section is mostly an annoyance that needs to be
> worked around, without providing any tangible benefit.
> 
> Unlike static subprog calls, global subprog calls are not safe to permit
> within the critical section, as the verifier does not explore them
> during verification, therefore whether the same lock will be taken
> again, or unlocked, cannot be ascertained.
> 
> Therefore, allow calling static subprogs within a bpf_spin_lock critical
> section, and only reject it in case the subprog linkage is global.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Looks good, thanks for this improvement. I had the same suggestion as
Yonghong in [0], and also left a question below.

[0]: https://lore.kernel.org/all/2e008ab1-44b8-4d1b-a86d-1f347d7630e6@linux.dev/

Acked-by: David Vernet <void@manifault.com>

> ---
>  kernel/bpf/verifier.c                                  | 10 +++++++---
>  tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 64fa188d00ad..f858c959753b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	if (subprog_is_global(env, subprog)) {
>  		const char *sub_name = subprog_name(env, subprog);
>  
> +		/* Only global subprogs cannot be called with a lock held. */
> +		if (env->cur_state->active_lock.ptr) {
> +			verbose(env, "function calls are not allowed while holding a lock\n");
> +			return -EINVAL;
> +		}
> +
>  		if (err) {
>  			verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
>  				subprog, sub_name);
> @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
>  
>  				if (env->cur_state->active_lock.ptr) {
>  					if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> -					    (insn->src_reg == BPF_PSEUDO_CALL) ||
>  					    (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>  					     (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
>  						verbose(env, "function calls are not allowed while holding a lock\n");
> @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
>  					return -EINVAL;
>  				}
>  process_bpf_exit_full:
> -				if (env->cur_state->active_lock.ptr &&
> -				    !in_rbtree_lock_required_cb(env)) {
> +				if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {

Can we do the same thing here for the RCU check below? It seems like the
exact same issue, as we're already allowed to call subprogs from within
an RCU read region, but the verifier will get confused and think we
haven't unlocked by the time we return to the caller.

Assuming that's the case, we can take care of it in a separate patch
set.

>  					verbose(env, "bpf_spin_unlock is missing\n");
>  					return -EINVAL;
>  				}
> diff --git a/tools/testing/selftests/bpf/progs/verifier_spin_lock.c b/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
> index 9c1aa69650f8..fb316c080c84 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_spin_lock.c
> @@ -330,7 +330,7 @@ l1_%=:	r7 = r0;					\
>  
>  SEC("cgroup/skb")
>  __description("spin_lock: test10 lock in subprog without unlock")
> -__failure __msg("unlock is missing")
> +__success
>  __failure_unpriv __msg_unpriv("")
>  __naked void lock_in_subprog_without_unlock(void)
>  {
> -- 
> 2.40.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Kumar Kartikeya Dwivedi]

On Sun, 4 Feb 2024 at 22:33, David Vernet <void@manifault.com> wrote:
>
> On Sun, Feb 04, 2024 at 12:02:05PM +0000, Kumar Kartikeya Dwivedi wrote:
> > Currently, calling any helpers, kfuncs, or subprogs except the graph
> > data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
> > is not allowed. One of the original motivations of this decision was to
> > force the BPF programmer's hand into keeping the bpf_spin_lock critical
> > section small, and to ensure the execution time of the program does not
> > increase due to lock waiting times. In addition to this, some of the
> > helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
> >
> > However, when it comes to subprog calls, atleast for static subprogs,
> > the verifier is able to explore their instructions during verification.
> > Therefore, it is similar in effect to having the same code inlined into
> > the critical section. Hence, not allowing static subprog calls in the
> > bpf_spin_lock critical section is mostly an annoyance that needs to be
> > worked around, without providing any tangible benefit.
> >
> > Unlike static subprog calls, global subprog calls are not safe to permit
> > within the critical section, as the verifier does not explore them
> > during verification, therefore whether the same lock will be taken
> > again, or unlocked, cannot be ascertained.
> >
> > Therefore, allow calling static subprogs within a bpf_spin_lock critical
> > section, and only reject it in case the subprog linkage is global.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> Looks good, thanks for this improvement. I had the same suggestion as
> Yonghong in [0], and also left a question below.
>
> [0]: https://lore.kernel.org/all/2e008ab1-44b8-4d1b-a86d-1f347d7630e6@linux.dev/
>
> Acked-by: David Vernet <void@manifault.com>
>
> > ---
> >  kernel/bpf/verifier.c                                  | 10 +++++++---
> >  tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 64fa188d00ad..f858c959753b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >       if (subprog_is_global(env, subprog)) {
> >               const char *sub_name = subprog_name(env, subprog);
> >
> > +             /* Only global subprogs cannot be called with a lock held. */
> > +             if (env->cur_state->active_lock.ptr) {
> > +                     verbose(env, "function calls are not allowed while holding a lock\n");
> > +                     return -EINVAL;
> > +             }
> > +
> >               if (err) {
> >                       verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
> >                               subprog, sub_name);
> > @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
> >
> >                               if (env->cur_state->active_lock.ptr) {
> >                                       if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
> > -                                         (insn->src_reg == BPF_PSEUDO_CALL) ||
> >                                           (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> >                                            (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
> >                                               verbose(env, "function calls are not allowed while holding a lock\n");
> > @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
> >                                       return -EINVAL;
> >                               }
> >  process_bpf_exit_full:
> > -                             if (env->cur_state->active_lock.ptr &&
> > -                                 !in_rbtree_lock_required_cb(env)) {
> > +                             if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
>
> Can we do the same thing here for the RCU check below? It seems like the
> exact same issue, as we're already allowed to call subprogs from within
> an RCU read region, but the verifier will get confused and think we
> haven't unlocked by the time we return to the caller.
>
> Assuming that's the case, we can take care of it in a separate patch
> set.

Makes sense, I'll send a separate patch for the RCU fix.
Thanks for the review.

Re: [PATCH bpf-next v1 1/2] bpf: Allow calling static subprogs while holding a bpf_spin_lock

[Yonghong Song]

On 2/4/24 2:10 PM, Kumar Kartikeya Dwivedi wrote:
> On Sun, 4 Feb 2024 at 22:33, David Vernet <void@manifault.com> wrote:
>> On Sun, Feb 04, 2024 at 12:02:05PM +0000, Kumar Kartikeya Dwivedi wrote:
>>> Currently, calling any helpers, kfuncs, or subprogs except the graph
>>> data structure (lists, rbtrees) API kfuncs while holding a bpf_spin_lock
>>> is not allowed. One of the original motivations of this decision was to
>>> force the BPF programmer's hand into keeping the bpf_spin_lock critical
>>> section small, and to ensure the execution time of the program does not
>>> increase due to lock waiting times. In addition to this, some of the
>>> helpers and kfuncs may be unsafe to call while holding a bpf_spin_lock.
>>>
>>> However, when it comes to subprog calls, atleast for static subprogs,
>>> the verifier is able to explore their instructions during verification.
>>> Therefore, it is similar in effect to having the same code inlined into
>>> the critical section. Hence, not allowing static subprog calls in the
>>> bpf_spin_lock critical section is mostly an annoyance that needs to be
>>> worked around, without providing any tangible benefit.
>>>
>>> Unlike static subprog calls, global subprog calls are not safe to permit
>>> within the critical section, as the verifier does not explore them
>>> during verification, therefore whether the same lock will be taken
>>> again, or unlocked, cannot be ascertained.
>>>
>>> Therefore, allow calling static subprogs within a bpf_spin_lock critical
>>> section, and only reject it in case the subprog linkage is global.
>>>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> Looks good, thanks for this improvement. I had the same suggestion as
>> Yonghong in [0], and also left a question below.
>>
>> [0]: https://lore.kernel.org/all/2e008ab1-44b8-4d1b-a86d-1f347d7630e6@linux.dev/
>>
>> Acked-by: David Vernet <void@manifault.com>
>>
>>> ---
>>>   kernel/bpf/verifier.c                                  | 10 +++++++---
>>>   tools/testing/selftests/bpf/progs/verifier_spin_lock.c |  2 +-
>>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 64fa188d00ad..f858c959753b 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -9493,6 +9493,12 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>        if (subprog_is_global(env, subprog)) {
>>>                const char *sub_name = subprog_name(env, subprog);
>>>
>>> +             /* Only global subprogs cannot be called with a lock held. */
>>> +             if (env->cur_state->active_lock.ptr) {
>>> +                     verbose(env, "function calls are not allowed while holding a lock\n");
>>> +                     return -EINVAL;
>>> +             }
>>> +
>>>                if (err) {
>>>                        verbose(env, "Caller passes invalid args into func#%d ('%s')\n",
>>>                                subprog, sub_name);
>>> @@ -17644,7 +17650,6 @@ static int do_check(struct bpf_verifier_env *env)
>>>
>>>                                if (env->cur_state->active_lock.ptr) {
>>>                                        if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) ||
>>> -                                         (insn->src_reg == BPF_PSEUDO_CALL) ||
>>>                                            (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
>>>                                             (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) {
>>>                                                verbose(env, "function calls are not allowed while holding a lock\n");
>>> @@ -17692,8 +17697,7 @@ static int do_check(struct bpf_verifier_env *env)
>>>                                        return -EINVAL;
>>>                                }
>>>   process_bpf_exit_full:
>>> -                             if (env->cur_state->active_lock.ptr &&
>>> -                                 !in_rbtree_lock_required_cb(env)) {
>>> +                             if (env->cur_state->active_lock.ptr && !env->cur_state->curframe) {
>> Can we do the same thing here for the RCU check below? It seems like the
>> exact same issue, as we're already allowed to call subprogs from within
>> an RCU read region, but the verifier will get confused and think we
>> haven't unlocked by the time we return to the caller.
>>
>> Assuming that's the case, we can take care of it in a separate patch
>> set.
> Makes sense, I'll send a separate patch for the RCU fix.
> Thanks for the review.

The following is what I recommended as well in another thread:

https://lore.kernel.org/bpf/20240131145454.86990-1-laoar.shao@gmail.com/T/#mff17cd64eeb1e17bd0e3e046fb52efeef9c86c25

>

[PATCH bpf-next v1 2/2] selftests/bpf: Add test for static subprog call in lock cs

[Kumar Kartikeya Dwivedi]

Add selftests for static subprog calls within bpf_spin_lock critical
section, and ensure we still reject global subprog calls. Also test the
case where a subprog call will unlock the caller's held lock, or the
caller will unlock a lock taken by a subprog call, ensuring correct
transfer of lock state across frames on exit.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/spin_lock.c      |  2 +
 .../selftests/bpf/progs/test_spin_lock.c      | 65 +++++++++++++++++++
 .../selftests/bpf/progs/test_spin_lock_fail.c | 44 +++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/spin_lock.c b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
index 18d451be57c8..6a4962ca0e5e 100644
--- a/tools/testing/selftests/bpf/prog_tests/spin_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
@@ -48,6 +48,8 @@ static struct {
 	{ "lock_id_mismatch_innermapval_kptr", "bpf_spin_unlock of different lock" },
 	{ "lock_id_mismatch_innermapval_global", "bpf_spin_unlock of different lock" },
 	{ "lock_id_mismatch_innermapval_mapval", "bpf_spin_unlock of different lock" },
+	{ "lock_global_subprog_call1", "function calls are not allowed while holding a lock" },
+	{ "lock_global_subprog_call2", "function calls are not allowed while holding a lock" },
 };
 
 static int match_regex(const char *pattern, const char *string)
diff --git a/tools/testing/selftests/bpf/progs/test_spin_lock.c b/tools/testing/selftests/bpf/progs/test_spin_lock.c
index b2440a0ff422..d8d77bdffd3d 100644
--- a/tools/testing/selftests/bpf/progs/test_spin_lock.c
+++ b/tools/testing/selftests/bpf/progs/test_spin_lock.c
@@ -101,4 +101,69 @@ int bpf_spin_lock_test(struct __sk_buff *skb)
 err:
 	return err;
 }
+
+struct bpf_spin_lock lockA __hidden SEC(".data.A");
+
+__noinline
+static int static_subprog(struct __sk_buff *ctx)
+{
+	volatile int ret = 0;
+
+	if (ctx->protocol)
+		return ret;
+	return ret + ctx->len;
+}
+
+__noinline
+static int static_subprog_lock(struct __sk_buff *ctx)
+{
+	volatile int ret = 0;
+
+	ret = static_subprog(ctx);
+	bpf_spin_lock(&lockA);
+	return ret + ctx->len;
+}
+
+__noinline
+static int static_subprog_unlock(struct __sk_buff *ctx)
+{
+	volatile int ret = 0;
+
+	ret = static_subprog(ctx);
+	bpf_spin_unlock(&lockA);
+	return ret + ctx->len;
+}
+
+SEC("tc")
+int lock_static_subprog_call(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	if (ctx->mark == 42)
+		ret = static_subprog(ctx);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
+SEC("tc")
+int lock_static_subprog_lock(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	ret = static_subprog_lock(ctx);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
+SEC("tc")
+int lock_static_subprog_unlock(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	ret = static_subprog_unlock(ctx);
+	return ret;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c b/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c
index 86cd183ef6dc..43f40c4fe241 100644
--- a/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c
+++ b/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c
@@ -201,4 +201,48 @@ CHECK(innermapval_mapval, &iv->lock, &v->lock);
 
 #undef CHECK
 
+__noinline
+int global_subprog(struct __sk_buff *ctx)
+{
+	volatile int ret = 0;
+
+	if (ctx->protocol)
+		ret += ctx->protocol;
+	return ret + ctx->mark;
+}
+
+__noinline
+static int static_subprog_call_global(struct __sk_buff *ctx)
+{
+	volatile int ret = 0;
+
+	if (ctx->protocol)
+		return ret;
+	return ret + ctx->len + global_subprog(ctx);
+}
+
+SEC("?tc")
+int lock_global_subprog_call1(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	if (ctx->mark == 42)
+		ret = global_subprog(ctx);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
+SEC("?tc")
+int lock_global_subprog_call2(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	if (ctx->mark == 42)
+		ret = static_subprog_call_global(ctx);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.40.1

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add test for static subprog call in lock cs

[Yonghong Song]

On 2/4/24 4:02 AM, Kumar Kartikeya Dwivedi wrote:
> Add selftests for static subprog calls within bpf_spin_lock critical
> section, and ensure we still reject global subprog calls. Also test the
> case where a subprog call will unlock the caller's held lock, or the
> caller will unlock a lock taken by a subprog call, ensuring correct
> transfer of lock state across frames on exit.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

LGTM with possible verifier message rewording from "function calls are
not allowed while holding a lock" to "global function calls are not
allowed ...".

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   .../selftests/bpf/prog_tests/spin_lock.c      |  2 +
>   .../selftests/bpf/progs/test_spin_lock.c      | 65 +++++++++++++++++++
>   .../selftests/bpf/progs/test_spin_lock_fail.c | 44 +++++++++++++
>   3 files changed, 111 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/spin_lock.c b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
> index 18d451be57c8..6a4962ca0e5e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/spin_lock.c
> +++ b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
> @@ -48,6 +48,8 @@ static struct {
>   	{ "lock_id_mismatch_innermapval_kptr", "bpf_spin_unlock of different lock" },
>   	{ "lock_id_mismatch_innermapval_global", "bpf_spin_unlock of different lock" },
>   	{ "lock_id_mismatch_innermapval_mapval", "bpf_spin_unlock of different lock" },
> +	{ "lock_global_subprog_call1", "function calls are not allowed while holding a lock" },
> +	{ "lock_global_subprog_call2", "function calls are not allowed while holding a lock" },
>   };
>   
>   static int match_regex(const char *pattern, const char *string)
[...]

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Add test for static subprog call in lock cs

[David Vernet]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Sun, Feb 04, 2024 at 12:02:06PM +0000, Kumar Kartikeya Dwivedi wrote:
> Add selftests for static subprog calls within bpf_spin_lock critical
> section, and ensure we still reject global subprog calls. Also test the
> case where a subprog call will unlock the caller's held lock, or the
> caller will unlock a lock taken by a subprog call, ensuring correct
> transfer of lock state across frames on exit.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Same nit as Yonghong again to just slightly improve the error message in
the verifier. Otherwise LGTM, thanks.

Acked-by: David Vernet <void@manifault.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]