[PATCH bpf-next v1 0/2] Global subprogs in RCU/{preempt,irq}-disabled sections

[PATCH bpf-next v1 0/2] Global subprogs in RCU/{preempt,irq}-disabled sections

[Kumar Kartikeya Dwivedi]

Small change to allow non-sleepable global subprogs in
RCU, preempt-disabled, and irq-disabled sections. For
now, we don't lift the limitation for locks as it requires
more analysis, and will do this one resilient spin locks
land.

This surfaced a bug where sleepable global subprogs were
allowed in RCU read sections, that has been fixed. Tests
have been added to cover various cases.

Kumar Kartikeya Dwivedi (2):
  bpf: Summarize sleepable global subprogs
  selftests/bpf: Test sleepable global subprogs in atomic contexts

 include/linux/bpf_verifier.h                  |  1 +
 kernel/bpf/verifier.c                         | 50 ++++++++++-----
 .../selftests/bpf/prog_tests/rcu_read_lock.c  |  2 +
 .../selftests/bpf/prog_tests/spin_lock.c      |  2 +
 tools/testing/selftests/bpf/progs/irq.c       | 62 ++++++++++++++++++-
 .../selftests/bpf/progs/preempt_lock.c        | 40 +++++++++++-
 .../selftests/bpf/progs/rcu_read_lock.c       | 38 ++++++++++++
 .../selftests/bpf/progs/test_spin_lock_fail.c | 40 ++++++++++++
 8 files changed, 219 insertions(+), 16 deletions(-)


base-commit: 0b9363131daf4227d5ae11ee677acdcfff06e938
-- 
2.43.5

[PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Kumar Kartikeya Dwivedi]

The verifier currently does not permit global subprog calls when a lock
is held, preemption is disabled, or when IRQs are disabled. This is
because we don't know whether the global subprog calls sleepable
functions or not.

In case of locks, there's an additional reason: functions called by the
global subprog may hold additional locks etc. The verifier won't know
while verifying the global subprog whether it was called in context
where a spin lock is already held by the program.

Perform summarization of the sleepable nature of a global subprog just
like changes_pkt_data and then allow calls to global subprogs for
non-sleepable ones from atomic context.

While making this change, I noticed that RCU read sections had no
protection against sleepable global subprog calls, include it in the
checks and fix this while we're at it.

Care needs to be taken to not allow global subprog calls when regular
bpf_spin_lock is held. When resilient spin locks is held, we want to
potentially have this check relaxed, but not for now.

Tests are included in the next patch to handle all special conditions.

Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |  1 +
 kernel/bpf/verifier.c        | 50 ++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index bbd013c38ff9..1b3cfa6cb720 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -667,6 +667,7 @@ struct bpf_subprog_info {
 	/* true if bpf_fastcall stack region is used by functions that can't be inlined */
 	bool keep_fastcall_stack: 1;
 	bool changes_pkt_data: 1;
+	bool sleepable: 1;
 
 	enum priv_stack_mode priv_stack_mode;
 	u8 arg_cnt;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index dcd0da4e62fc..e3560d19d513 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10317,23 +10317,18 @@ 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_locks) {
 			verbose(env, "global function calls are not allowed while holding a lock,\n"
 				     "use static function instead\n");
 			return -EINVAL;
 		}
 
-		/* Only global subprogs cannot be called with preemption disabled. */
-		if (env->cur_state->active_preempt_locks) {
-			verbose(env, "global function calls are not allowed with preemption disabled,\n"
-				     "use static function instead\n");
-			return -EINVAL;
-		}
-
-		if (env->cur_state->active_irq_id) {
-			verbose(env, "global function calls are not allowed with IRQs disabled,\n"
-				     "use static function instead\n");
+		if (env->subprog_info[subprog].sleepable &&
+		    (env->cur_state->active_rcu_lock || env->cur_state->active_preempt_locks ||
+		     env->cur_state->active_irq_id || !in_sleepable(env))) {
+			verbose(env, "global functions that may sleep are not allowed in non-sleepable context,\n"
+				     "i.e., in a RCU/IRQ/preempt-disabled section, or in\n"
+				     "a non-sleepable BPF program context\n");
 			return -EINVAL;
 		}
 
@@ -16703,6 +16698,14 @@ static void mark_subprog_changes_pkt_data(struct bpf_verifier_env *env, int off)
 	subprog->changes_pkt_data = true;
 }
 
+static void mark_subprog_sleepable(struct bpf_verifier_env *env, int off)
+{
+	struct bpf_subprog_info *subprog;
+
+	subprog = find_containing_subprog(env, off);
+	subprog->sleepable = true;
+}
+
 /* 't' is an index of a call-site.
  * 'w' is a callee entry point.
  * Eventually this function would be called when env->cfg.insn_state[w] == EXPLORED.
@@ -16716,6 +16719,7 @@ static void merge_callee_effects(struct bpf_verifier_env *env, int t, int w)
 	caller = find_containing_subprog(env, t);
 	callee = find_containing_subprog(env, w);
 	caller->changes_pkt_data |= callee->changes_pkt_data;
+	caller->sleepable |= callee->sleepable;
 }
 
 /* non-recursive DFS pseudo code
@@ -17183,9 +17187,20 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 			mark_prune_point(env, t);
 			mark_jmp_point(env, t);
 		}
-		if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
-			mark_subprog_changes_pkt_data(env, t);
-		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
+		if (bpf_helper_call(insn)) {
+			const struct bpf_func_proto *fp;
+
+			ret = get_helper_proto(env, insn->imm, &fp);
+			/* If called in a non-sleepable context program will be
+			 * rejected anyway, so we should end up with precise
+			 * sleepable marks on subprogs, except for dead code
+			 * elimination.
+			 */
+			if (ret == 0 && fp->might_sleep)
+				mark_subprog_sleepable(env, t);
+			if (bpf_helper_changes_pkt_data(insn->imm))
+				mark_subprog_changes_pkt_data(env, t);
+		} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
 			struct bpf_kfunc_call_arg_meta meta;
 
 			ret = fetch_kfunc_meta(env, insn, &meta, NULL);
@@ -17204,6 +17219,13 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 				 */
 				mark_force_checkpoint(env, t);
 			}
+			/* Same as helpers, if called in a non-sleepable context
+			 * program will be rejected anyway, so we should end up
+			 * with precise sleepable marks on subprogs, except for
+			 * dead code elimination.
+			 */
+			if (ret == 0 && is_kfunc_sleepable(&meta))
+				mark_subprog_sleepable(env, t);
 		}
 		return visit_func_call_insn(t, insns, env, insn->src_reg == BPF_PSEUDO_CALL);
 
-- 
2.43.5

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Eduard Zingerman]

On Fri, 2025-02-28 at 08:28 -0800, Kumar Kartikeya Dwivedi wrote:
> The verifier currently does not permit global subprog calls when a lock
> is held, preemption is disabled, or when IRQs are disabled. This is
> because we don't know whether the global subprog calls sleepable
> functions or not.
> 
> In case of locks, there's an additional reason: functions called by the
> global subprog may hold additional locks etc. The verifier won't know
> while verifying the global subprog whether it was called in context
> where a spin lock is already held by the program.
> 
> Perform summarization of the sleepable nature of a global subprog just
> like changes_pkt_data and then allow calls to global subprogs for
> non-sleepable ones from atomic context.
> 
> While making this change, I noticed that RCU read sections had no
> protection against sleepable global subprog calls, include it in the
> checks and fix this while we're at it.
> 
> Care needs to be taken to not allow global subprog calls when regular
> bpf_spin_lock is held. When resilient spin locks is held, we want to
> potentially have this check relaxed, but not for now.
> 
> Tests are included in the next patch to handle all special conditions.
> 
> Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

I think this change also has to deal with freplace for sleepable
sub-programs, e.g. see verifier.c:bpf_check_attach_target(),
part dealing with `tgt_changes_pkt_data`.

Other than that the logic seems ok.

[...]

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Kumar Kartikeya Dwivedi]

On Fri, 28 Feb 2025 at 21:42, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-02-28 at 08:28 -0800, Kumar Kartikeya Dwivedi wrote:
> > The verifier currently does not permit global subprog calls when a lock
> > is held, preemption is disabled, or when IRQs are disabled. This is
> > because we don't know whether the global subprog calls sleepable
> > functions or not.
> >
> > In case of locks, there's an additional reason: functions called by the
> > global subprog may hold additional locks etc. The verifier won't know
> > while verifying the global subprog whether it was called in context
> > where a spin lock is already held by the program.
> >
> > Perform summarization of the sleepable nature of a global subprog just
> > like changes_pkt_data and then allow calls to global subprogs for
> > non-sleepable ones from atomic context.
> >
> > While making this change, I noticed that RCU read sections had no
> > protection against sleepable global subprog calls, include it in the
> > checks and fix this while we're at it.
> >
> > Care needs to be taken to not allow global subprog calls when regular
> > bpf_spin_lock is held. When resilient spin locks is held, we want to
> > potentially have this check relaxed, but not for now.
> >
> > Tests are included in the next patch to handle all special conditions.
> >
> > Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
>
> I think this change also has to deal with freplace for sleepable
> sub-programs, e.g. see verifier.c:bpf_check_attach_target(),
> part dealing with `tgt_changes_pkt_data`.
>
> Other than that the logic seems ok.

Ah, good catch. Let me fix that and add a test to check it.

>
> [...]
>

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Andrii Nakryiko]

On Fri, Feb 28, 2025 at 8:29 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> The verifier currently does not permit global subprog calls when a lock
> is held, preemption is disabled, or when IRQs are disabled. This is
> because we don't know whether the global subprog calls sleepable
> functions or not.
>
> In case of locks, there's an additional reason: functions called by the
> global subprog may hold additional locks etc. The verifier won't know
> while verifying the global subprog whether it was called in context
> where a spin lock is already held by the program.
>
> Perform summarization of the sleepable nature of a global subprog just
> like changes_pkt_data and then allow calls to global subprogs for
> non-sleepable ones from atomic context.
>
> While making this change, I noticed that RCU read sections had no
> protection against sleepable global subprog calls, include it in the
> checks and fix this while we're at it.
>
> Care needs to be taken to not allow global subprog calls when regular
> bpf_spin_lock is held. When resilient spin locks is held, we want to
> potentially have this check relaxed, but not for now.
>
> Tests are included in the next patch to handle all special conditions.
>
> Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/bpf_verifier.h |  1 +
>  kernel/bpf/verifier.c        | 50 ++++++++++++++++++++++++++----------
>  2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index bbd013c38ff9..1b3cfa6cb720 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -667,6 +667,7 @@ struct bpf_subprog_info {
>         /* true if bpf_fastcall stack region is used by functions that can't be inlined */
>         bool keep_fastcall_stack: 1;
>         bool changes_pkt_data: 1;
> +       bool sleepable: 1;
>
>         enum priv_stack_mode priv_stack_mode;
>         u8 arg_cnt;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index dcd0da4e62fc..e3560d19d513 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10317,23 +10317,18 @@ 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_locks) {
>                         verbose(env, "global function calls are not allowed while holding a lock,\n"
>                                      "use static function instead\n");
>                         return -EINVAL;
>                 }
>
> -               /* Only global subprogs cannot be called with preemption disabled. */
> -               if (env->cur_state->active_preempt_locks) {
> -                       verbose(env, "global function calls are not allowed with preemption disabled,\n"
> -                                    "use static function instead\n");
> -                       return -EINVAL;
> -               }
> -
> -               if (env->cur_state->active_irq_id) {
> -                       verbose(env, "global function calls are not allowed with IRQs disabled,\n"
> -                                    "use static function instead\n");
> +               if (env->subprog_info[subprog].sleepable &&
> +                   (env->cur_state->active_rcu_lock || env->cur_state->active_preempt_locks ||
> +                    env->cur_state->active_irq_id || !in_sleepable(env))) {
> +                       verbose(env, "global functions that may sleep are not allowed in non-sleepable context,\n"
> +                                    "i.e., in a RCU/IRQ/preempt-disabled section, or in\n"
> +                                    "a non-sleepable BPF program context\n");
>                         return -EINVAL;
>                 }
>
> @@ -16703,6 +16698,14 @@ static void mark_subprog_changes_pkt_data(struct bpf_verifier_env *env, int off)
>         subprog->changes_pkt_data = true;
>  }
>
> +static void mark_subprog_sleepable(struct bpf_verifier_env *env, int off)
> +{
> +       struct bpf_subprog_info *subprog;
> +
> +       subprog = find_containing_subprog(env, off);
> +       subprog->sleepable = true;
> +}
> +
>  /* 't' is an index of a call-site.
>   * 'w' is a callee entry point.
>   * Eventually this function would be called when env->cfg.insn_state[w] == EXPLORED.
> @@ -16716,6 +16719,7 @@ static void merge_callee_effects(struct bpf_verifier_env *env, int t, int w)
>         caller = find_containing_subprog(env, t);
>         callee = find_containing_subprog(env, w);
>         caller->changes_pkt_data |= callee->changes_pkt_data;
> +       caller->sleepable |= callee->sleepable;
>  }
>
>  /* non-recursive DFS pseudo code
> @@ -17183,9 +17187,20 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>                         mark_prune_point(env, t);
>                         mark_jmp_point(env, t);
>                 }
> -               if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
> -                       mark_subprog_changes_pkt_data(env, t);
> -               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +               if (bpf_helper_call(insn)) {
> +                       const struct bpf_func_proto *fp;
> +
> +                       ret = get_helper_proto(env, insn->imm, &fp);
> +                       /* If called in a non-sleepable context program will be
> +                        * rejected anyway, so we should end up with precise
> +                        * sleepable marks on subprogs, except for dead code
> +                        * elimination.

TBH, I'm worried that we are regressing to doing all these side effect
analyses disregarding dead code elimination. It's not something
hypothetical to have an .rodata variable controlling whether, say, to
do bpf_probe_read_user() (non-sleepable) vs bpf_copy_from_user()
(sleepable) inside global subprog, depending on some outside
configuration (e.g., whether we'll be doing SEC("iter.s/task") or it's
actually profiler logic called inside SEC("perf_event"), all
controlled by user-space). We do have use cases like this in
production already, and this dead code elimination is important in
such cases. Probably can be worked around with more global functions
and stuff like that, but still, it's worrying we are giving up on such
an important part of the BPF CO-RE approach - disabling parts of code
"dynamically" before loading BPF programs.

> +                        */
> +                       if (ret == 0 && fp->might_sleep)
> +                               mark_subprog_sleepable(env, t);
> +                       if (bpf_helper_changes_pkt_data(insn->imm))
> +                               mark_subprog_changes_pkt_data(env, t);
> +               } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>                         struct bpf_kfunc_call_arg_meta meta;
>
>                         ret = fetch_kfunc_meta(env, insn, &meta, NULL);
> @@ -17204,6 +17219,13 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>                                  */
>                                 mark_force_checkpoint(env, t);
>                         }
> +                       /* Same as helpers, if called in a non-sleepable context
> +                        * program will be rejected anyway, so we should end up
> +                        * with precise sleepable marks on subprogs, except for
> +                        * dead code elimination.
> +                        */
> +                       if (ret == 0 && is_kfunc_sleepable(&meta))
> +                               mark_subprog_sleepable(env, t);
>                 }
>                 return visit_func_call_insn(t, insns, env, insn->src_reg == BPF_PSEUDO_CALL);
>
> --
> 2.43.5
>

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Eduard Zingerman]

On Fri, 2025-02-28 at 15:18 -0800, Andrii Nakryiko wrote:

[...]

> >  /* non-recursive DFS pseudo code
> > @@ -17183,9 +17187,20 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
> >                         mark_prune_point(env, t);
> >                         mark_jmp_point(env, t);
> >                 }
> > -               if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
> > -                       mark_subprog_changes_pkt_data(env, t);
> > -               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > +               if (bpf_helper_call(insn)) {
> > +                       const struct bpf_func_proto *fp;
> > +
> > +                       ret = get_helper_proto(env, insn->imm, &fp);
> > +                       /* If called in a non-sleepable context program will be
> > +                        * rejected anyway, so we should end up with precise
> > +                        * sleepable marks on subprogs, except for dead code
> > +                        * elimination.
> 
> TBH, I'm worried that we are regressing to doing all these side effect
> analyses disregarding dead code elimination. It's not something
> hypothetical to have an .rodata variable controlling whether, say, to
> do bpf_probe_read_user() (non-sleepable) vs bpf_copy_from_user()
> (sleepable) inside global subprog, depending on some outside
> configuration (e.g., whether we'll be doing SEC("iter.s/task") or it's
> actually profiler logic called inside SEC("perf_event"), all
> controlled by user-space). We do have use cases like this in
> production already, and this dead code elimination is important in
> such cases. Probably can be worked around with more global functions
> and stuff like that, but still, it's worrying we are giving up on such
> an important part of the BPF CO-RE approach - disabling parts of code
> "dynamically" before loading BPF programs.

There were two alternatives on the table last time:
- add support for tags on global functions;
- verify global subprogram call tree in post-order,
  in order to have the flags ready when needed.

Both were rejected back than.
But we still can reconsider :)

> > +                        */
> > +                       if (ret == 0 && fp->might_sleep)
> > +                               mark_subprog_sleepable(env, t);
> > +                       if (bpf_helper_changes_pkt_data(insn->imm))
> > +                               mark_subprog_changes_pkt_data(env, t);
> > +               } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> >                         struct bpf_kfunc_call_arg_meta meta;
> > 
> >                         ret = fetch_kfunc_meta(env, insn, &meta, NULL);

[...]

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Andrii Nakryiko]

On Fri, Feb 28, 2025 at 3:23 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-02-28 at 15:18 -0800, Andrii Nakryiko wrote:
>
> [...]
>
> > >  /* non-recursive DFS pseudo code
> > > @@ -17183,9 +17187,20 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
> > >                         mark_prune_point(env, t);
> > >                         mark_jmp_point(env, t);
> > >                 }
> > > -               if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
> > > -                       mark_subprog_changes_pkt_data(env, t);
> > > -               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > > +               if (bpf_helper_call(insn)) {
> > > +                       const struct bpf_func_proto *fp;
> > > +
> > > +                       ret = get_helper_proto(env, insn->imm, &fp);
> > > +                       /* If called in a non-sleepable context program will be
> > > +                        * rejected anyway, so we should end up with precise
> > > +                        * sleepable marks on subprogs, except for dead code
> > > +                        * elimination.
> >
> > TBH, I'm worried that we are regressing to doing all these side effect
> > analyses disregarding dead code elimination. It's not something
> > hypothetical to have an .rodata variable controlling whether, say, to
> > do bpf_probe_read_user() (non-sleepable) vs bpf_copy_from_user()
> > (sleepable) inside global subprog, depending on some outside
> > configuration (e.g., whether we'll be doing SEC("iter.s/task") or it's
> > actually profiler logic called inside SEC("perf_event"), all
> > controlled by user-space). We do have use cases like this in
> > production already, and this dead code elimination is important in
> > such cases. Probably can be worked around with more global functions
> > and stuff like that, but still, it's worrying we are giving up on such
> > an important part of the BPF CO-RE approach - disabling parts of code
> > "dynamically" before loading BPF programs.
>
> There were two alternatives on the table last time:
> - add support for tags on global functions;

I was supportive of this, I believe

> - verify global subprogram call tree in post-order,
>   in order to have the flags ready when needed.

Remind me of the details here? we'd start validating the main prog,
suspend that process when encountering global func, go validate global
func, once done, come back to main prog, right?

Alternatively, we could mark expected properties (restrictions) of
global subprogs as we encounter them, right? E.g, if we come to global
func call inside rcu_read_{lock,unlock}() region, we'd mark it
internally as "needs to be non-sleepable".

>
> Both were rejected back than.
> But we still can reconsider :)
>

yep, though I'm not really feeling hopeful

> > > +                        */
> > > +                       if (ret == 0 && fp->might_sleep)
> > > +                               mark_subprog_sleepable(env, t);
> > > +                       if (bpf_helper_changes_pkt_data(insn->imm))
> > > +                               mark_subprog_changes_pkt_data(env, t);
> > > +               } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > >                         struct bpf_kfunc_call_arg_meta meta;
> > >
> > >                         ret = fetch_kfunc_meta(env, insn, &meta, NULL);
>
> [...]
>

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Eduard Zingerman]

On Fri, 2025-02-28 at 15:34 -0800, Andrii Nakryiko wrote:

[...]

> > There were two alternatives on the table last time:
> > - add support for tags on global functions;
> 
> I was supportive of this, I believe
> 
> > - verify global subprogram call tree in post-order,
> >   in order to have the flags ready when needed.
> 
> Remind me of the details here? we'd start validating the main prog,
> suspend that process when encountering global func, go validate global
> func, once done, come back to main prog, right?

Yes.
The tree can't be built statically if we account for dead code
elimination, as post-order might change.

> Alternatively, we could mark expected properties (restrictions) of
> global subprogs as we encounter them, right? E.g, if we come to global
> func call inside rcu_read_{lock,unlock}() region, we'd mark it
> internally as "needs to be non-sleepable".

For situation like below, suppose verification order is main, foo,
bar, buz:
- main() sleepable
  - foo()
  - bar()
- foo():
  - buz()
- bar():
  - foo() while holding lock
- buz():
  - calls something sleepable

I think, to handle this the call-tree needs to be built on the main
verification pass, and then checked for sleepable.
But that won't work for changes_pkt_data, as verdict has to be known
right-away to decide whether to invalidate packet pointers.

[...]

Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Kumar Kartikeya Dwivedi]

On Sat, 1 Mar 2025 at 00:57, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-02-28 at 15:34 -0800, Andrii Nakryiko wrote:
>
> [...]
>
> > > There were two alternatives on the table last time:
> > > - add support for tags on global functions;
> >
> > I was supportive of this, I believe
> >
> > > - verify global subprogram call tree in post-order,
> > >   in order to have the flags ready when needed.
> >
> > Remind me of the details here? we'd start validating the main prog,
> > suspend that process when encountering global func, go validate global
> > func, once done, come back to main prog, right?
>
> Yes.
> The tree can't be built statically if we account for dead code
> elimination, as post-order might change.
>
> > Alternatively, we could mark expected properties (restrictions) of
> > global subprogs as we encounter them, right? E.g, if we come to global
> > func call inside rcu_read_{lock,unlock}() region, we'd mark it
> > internally as "needs to be non-sleepable".
>
> For situation like below, suppose verification order is main, foo,
> bar, buz:
> - main() sleepable
>   - foo()
>   - bar()
> - foo():
>   - buz()
> - bar():
>   - foo() while holding lock
> - buz():
>   - calls something sleepable
>
> I think, to handle this the call-tree needs to be built on the main
> verification pass, and then checked for sleepable.
> But that won't work for changes_pkt_data, as verdict has to be known
> right-away to decide whether to invalidate packet pointers.
>

I know over-conservative marking in presence of possible DCE is
non-ideal (that's why I put in the comment, so we revisit it later),
I'm getting the sense from this thread that either option is a lot
more work/complexity, or insufficient.
Except for possibly taggings things properly, but that's been nipped in the bud.

So I'm going to prepare a v2 addressing Eduard's comments, and if we
reach a consensus, I can follow up to address both changes_pkt_data
and sleepable global subprogs.

> [...]
>

[PATCH bpf-next v1 2/2] selftests/bpf: Test sleepable global subprogs in atomic contexts

[Kumar Kartikeya Dwivedi]

Add tests for rejecting sleepable and accepting non-sleepable global
function calls in atomic contexts. For spin locks, we still reject
all global function calls. Once resilient spin locks land, we will
carefully lift in cases where we deem it safe.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/rcu_read_lock.c  |  2 +
 .../selftests/bpf/prog_tests/spin_lock.c      |  2 +
 tools/testing/selftests/bpf/progs/irq.c       | 62 ++++++++++++++++++-
 .../selftests/bpf/progs/preempt_lock.c        | 40 +++++++++++-
 .../selftests/bpf/progs/rcu_read_lock.c       | 38 ++++++++++++
 .../selftests/bpf/progs/test_spin_lock_fail.c | 40 ++++++++++++
 6 files changed, 182 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
index ebe0c12b5536..2a2e3a3b4c20 100644
--- a/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/rcu_read_lock.c
@@ -81,6 +81,8 @@ static const char * const inproper_region_tests[] = {
 	"nested_rcu_region",
 	"rcu_read_lock_global_subprog_lock",
 	"rcu_read_lock_global_subprog_unlock",
+	"rcu_read_lock_sleepable_helper_global_subprog",
+	"rcu_read_lock_sleepable_kfunc_global_subprog",
 };
 
 static void test_inproper_region(void)
diff --git a/tools/testing/selftests/bpf/prog_tests/spin_lock.c b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
index 2b0068742ef9..4652c44a0346 100644
--- a/tools/testing/selftests/bpf/prog_tests/spin_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
@@ -50,6 +50,8 @@ static struct {
 	{ "lock_id_mismatch_innermapval_mapval", "bpf_spin_unlock of different lock" },
 	{ "lock_global_subprog_call1", "global function calls are not allowed while holding a lock" },
 	{ "lock_global_subprog_call2", "global function calls are not allowed while holding a lock" },
+	{ "lock_global_sleepable_helper_subprog", "global function calls are not allowed while holding a lock" },
+	{ "lock_global_sleepable_kfunc_subprog", "global 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/irq.c b/tools/testing/selftests/bpf/progs/irq.c
index b0b53d980964..e5e19f96faa0 100644
--- a/tools/testing/selftests/bpf/progs/irq.c
+++ b/tools/testing/selftests/bpf/progs/irq.c
@@ -222,7 +222,7 @@ int __noinline global_local_irq_balance(void)
 }
 
 SEC("?tc")
-__failure __msg("global function calls are not allowed with IRQs disabled")
+__success
 int irq_global_subprog(struct __sk_buff *ctx)
 {
 	unsigned long flags;
@@ -441,4 +441,64 @@ int irq_ooo_refs_array(struct __sk_buff *ctx)
 	return 0;
 }
 
+int __noinline
+global_sleepable_helper_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user(&i, sizeof(i), NULL);
+	return i;
+}
+
+int __noinline
+global_sleepable_kfunc_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user_str(&i, sizeof(i), NULL, 0);
+	return i;
+}
+
+int __noinline
+global_subprog(int i)
+{
+	if (i)
+		bpf_printk("%p", &i);
+	return i;
+}
+
+SEC("?syscall")
+__success
+int irq_non_sleepable_global_subprog(void *ctx)
+{
+	unsigned long flags;
+
+	bpf_local_irq_save(&flags);
+	global_subprog(0);
+	bpf_local_irq_restore(&flags);
+	return 0;
+}
+
+SEC("?syscall")
+__failure __msg("global functions that may sleep are not allowed in non-sleepable context")
+int irq_sleepable_helper_global_subprog(void *ctx)
+{
+	unsigned long flags;
+
+	bpf_local_irq_save(&flags);
+	global_sleepable_helper_subprog(0);
+	bpf_local_irq_restore(&flags);
+	return 0;
+}
+
+SEC("?syscall")
+__failure __msg("global functions that may sleep are not allowed in non-sleepable context")
+int irq_sleepable_kfunc_global_subprog(void *ctx)
+{
+	unsigned long flags;
+
+	bpf_local_irq_save(&flags);
+	global_sleepable_kfunc_subprog(0);
+	bpf_local_irq_restore(&flags);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/preempt_lock.c b/tools/testing/selftests/bpf/progs/preempt_lock.c
index 6c5797bf0ead..c3bb7918442e 100644
--- a/tools/testing/selftests/bpf/progs/preempt_lock.c
+++ b/tools/testing/selftests/bpf/progs/preempt_lock.c
@@ -134,7 +134,7 @@ int __noinline preempt_global_subprog(void)
 }
 
 SEC("?tc")
-__failure __msg("global function calls are not allowed with preemption disabled")
+__success
 int preempt_global_subprog_test(struct __sk_buff *ctx)
 {
 	preempt_disable();
@@ -143,4 +143,42 @@ int preempt_global_subprog_test(struct __sk_buff *ctx)
 	return 0;
 }
 
+int __noinline
+global_sleepable_helper_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user(&i, sizeof(i), NULL);
+	return i;
+}
+
+int __noinline
+global_sleepable_kfunc_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user_str(&i, sizeof(i), NULL, 0);
+	return i;
+}
+
+SEC("?syscall")
+__failure __msg("global functions that may sleep are not allowed in non-sleepable context")
+int preempt_global_sleepable_helper_subprog(struct __sk_buff *ctx)
+{
+	preempt_disable();
+	if (ctx->mark)
+		global_sleepable_helper_subprog(ctx->mark);
+	preempt_enable();
+	return 0;
+}
+
+SEC("?syscall")
+__failure __msg("global functions that may sleep are not allowed in non-sleepable context")
+int preempt_global_sleepable_kfunc_subprog(struct __sk_buff *ctx)
+{
+	preempt_disable();
+	if (ctx->mark)
+		global_sleepable_kfunc_subprog(ctx->mark);
+	preempt_enable();
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/rcu_read_lock.c b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
index ab3a532b7dd6..f7d2bdaed612 100644
--- a/tools/testing/selftests/bpf/progs/rcu_read_lock.c
+++ b/tools/testing/selftests/bpf/progs/rcu_read_lock.c
@@ -439,3 +439,41 @@ int rcu_read_lock_global_subprog_unlock(void *ctx)
 	ret += global_subprog_unlock(ret);
 	return 0;
 }
+
+int __noinline
+global_sleepable_helper_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user(&i, sizeof(i), NULL);
+	return i;
+}
+
+int __noinline
+global_sleepable_kfunc_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user_str(&i, sizeof(i), NULL, 0);
+	return i;
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int rcu_read_lock_sleepable_helper_global_subprog(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_lock();
+	ret += global_sleepable_helper_subprog(ret);
+	bpf_rcu_read_unlock();
+	return 0;
+}
+
+SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
+int rcu_read_lock_sleepable_kfunc_global_subprog(void *ctx)
+{
+	volatile int ret = 0;
+
+	bpf_rcu_read_lock();
+	ret += global_sleepable_kfunc_subprog(ret);
+	bpf_rcu_read_unlock();
+	return 0;
+}
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 1c8b678e2e9a..278ce14e3470 100644
--- a/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c
+++ b/tools/testing/selftests/bpf/progs/test_spin_lock_fail.c
@@ -245,4 +245,44 @@ int lock_global_subprog_call2(struct __sk_buff *ctx)
 	return ret;
 }
 
+int __noinline
+global_sleepable_helper_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user(&i, sizeof(i), NULL);
+	return i;
+}
+
+int __noinline
+global_sleepable_kfunc_subprog(int i)
+{
+	if (i)
+		bpf_copy_from_user_str(&i, sizeof(i), NULL, 0);
+	return i;
+}
+
+SEC("?syscall")
+int lock_global_sleepable_helper_subprog(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	if (ctx->mark == 42)
+		ret = global_sleepable_helper_subprog(ctx->mark);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
+SEC("?syscall")
+int lock_global_sleepable_kfunc_subprog(struct __sk_buff *ctx)
+{
+	int ret = 0;
+
+	bpf_spin_lock(&lockA);
+	if (ctx->mark == 42)
+		ret = global_sleepable_kfunc_subprog(ctx->mark);
+	bpf_spin_unlock(&lockA);
+	return ret;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.5

Re: [PATCH bpf-next v1 2/2] selftests/bpf: Test sleepable global subprogs in atomic contexts

[Eduard Zingerman]

On Fri, 2025-02-28 at 08:28 -0800, Kumar Kartikeya Dwivedi wrote:
> Add tests for rejecting sleepable and accepting non-sleepable global
> function calls in atomic contexts. For spin locks, we still reject
> all global function calls. Once resilient spin locks land, we will
> carefully lift in cases where we deem it safe.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Maybe add one more layer in some of the tests?
E.g. call non-sleepable global from sleepable global and vice-versa?

[...]