[PATCH v3 bpf-next 0/3] bpf: Reject attaching fexit to __noreturn functions

[PATCH v3 bpf-next 0/3] bpf: Reject attaching fexit to __noreturn functions

[Yafang Shao]

Attaching fexit probes to functions marked with __noreturn may lead to
unpredictable behavior. To avoid this, we will reject attaching probes to
such functions. Currently, there is no ideal solution, so we will hardcode
a check for all __noreturn functions. Since objtool already handles
this, we will leverage its implementation.

Once a more robust solution is found, this workaround can be removed.

v2->v3:
- Remove the need for objtool (Alexei)

v1->v2: https://lore.kernel.org/bpf/20250223062735.3341-1-laoar.shao@gmail.com/
- keep tools/objtool/noreturns.h as is (Josh)
- Add noreturns.h to objtool/sync-check.sh (Josh)
- Add verbose for the reject and simplify the test case (Song)

v1: https://lore.kernel.org/bpf/20250211023359.1570-1-laoar.shao@gmail.com/

Yafang Shao (2):
  bpf: Reject attaching fexit to functions annotated with __noreturn
  selftests/bpf: Add selftest for attaching fexit to __noreturn
    functions

 kernel/bpf/verifier.c                         | 57 +++++++++++++++++++
 .../bpf/prog_tests/fexit_noreturns.c          |  9 +++
 .../selftests/bpf/progs/fexit_noreturns.c     | 15 +++++
 3 files changed, 81 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fexit_noreturns.c
 create mode 100644 tools/testing/selftests/bpf/progs/fexit_noreturns.c

-- 
2.43.5

[PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Yafang Shao]

If we attach fexit to a function annotated with __noreturn, it will
cause an issue that the bpf trampoline image will be left over even if
the bpf link has been destroyed. Take attaching do_exit() for example. The
fexit works as follows,

  bpf_trampoline
  + __bpf_tramp_enter
    + percpu_ref_get(&tr->pcref);

  + call do_exit()

  + __bpf_tramp_exit
    + percpu_ref_put(&tr->pcref);

Since do_exit() never returns, the refcnt of the trampoline image is
never decremented, preventing it from being freed. That can be verified
with as follows,

  $ bpftool link show                                   <<<< nothing output
  $ grep "bpf_trampoline_[0-9]" /proc/kallsyms
  ffffffffc04cb000 t bpf_trampoline_6442526459    [bpf] <<<< leftover

With this change, attaching fexit probes to functions like do_exit() will
be rejected.

$ ./fexit
libbpf: prog 'fexit': BPF program load failed: -EINVAL
libbpf: prog 'fexit': -- BEGIN PROG LOAD LOG --
Attaching fexit to __noreturn functions is rejected.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/bpf/verifier.c | 57 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9971c03adfd5..6b0d3ed7dbd9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22841,6 +22841,59 @@ BTF_ID(func, __rcu_read_unlock)
 #endif
 BTF_SET_END(btf_id_deny)
 
+/* Functions annotated with __noreturn are denied. Currently, we must manually
+ * list all __noreturn functions here. Once a more robust solution is
+ * implemented, this workaround can be removed.
+ */
+BTF_SET_START(fexit_deny)
+#define NORETURN(fn) BTF_ID(func, fn)
+NORETURN(__fortify_panic)
+NORETURN(__ia32_sys_exit)
+NORETURN(__ia32_sys_exit_group)
+NORETURN(__kunit_abort)
+NORETURN(__module_put_and_kthread_exit)
+NORETURN(__stack_chk_fail)
+NORETURN(__tdx_hypercall_failed)
+NORETURN(__ubsan_handle_builtin_unreachable)
+NORETURN(__x64_sys_exit)
+NORETURN(__x64_sys_exit_group)
+NORETURN(arch_cpu_idle_dead)
+NORETURN(bch2_trans_in_restart_error)
+NORETURN(bch2_trans_restart_error)
+NORETURN(bch2_trans_unlocked_error)
+NORETURN(cpu_bringup_and_idle)
+NORETURN(cpu_startup_entry)
+NORETURN(do_exit)
+NORETURN(do_group_exit)
+NORETURN(do_task_dead)
+NORETURN(ex_handler_msr_mce)
+NORETURN(hlt_play_dead)
+NORETURN(hv_ghcb_terminate)
+NORETURN(kthread_complete_and_exit)
+NORETURN(kthread_exit)
+NORETURN(kunit_try_catch_throw)
+NORETURN(machine_real_restart)
+NORETURN(make_task_dead)
+NORETURN(mpt_halt_firmware)
+NORETURN(nmi_panic_self_stop)
+NORETURN(panic)
+NORETURN(panic_smp_self_stop)
+NORETURN(rest_init)
+NORETURN(rewind_stack_and_make_dead)
+NORETURN(rust_begin_unwind)
+NORETURN(rust_helper_BUG)
+NORETURN(sev_es_terminate)
+NORETURN(snp_abort)
+NORETURN(start_kernel)
+NORETURN(stop_this_cpu)
+NORETURN(usercopy_abort)
+NORETURN(x86_64_start_kernel)
+NORETURN(x86_64_start_reservations)
+NORETURN(xen_cpu_bringup_again)
+NORETURN(xen_start_kernel)
+#undef NORETURN
+BTF_SET_END(fexit_deny)
+
 static bool can_be_sleepable(struct bpf_prog *prog)
 {
 	if (prog->type == BPF_PROG_TYPE_TRACING) {
@@ -22929,6 +22982,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	} else if (prog->type == BPF_PROG_TYPE_TRACING &&
 		   btf_id_set_contains(&btf_id_deny, btf_id)) {
 		return -EINVAL;
+	} else if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
+		   btf_id_set_contains(&fexit_deny, btf_id)) {
+		verbose(env, "Attaching fexit to __noreturn functions is rejected.\n");
+		return -EINVAL;
 	}
 
 	key = bpf_trampoline_compute_key(tgt_prog, prog->aux->attach_btf, btf_id);
-- 
2.43.5

[PATCH v3 2/2] selftests/bpf: Add selftest for attaching fexit to __noreturn functions

[Yafang Shao]

The reuslt:

  $ tools/testing/selftests/bpf/test_progs --name=fexit_noreturns
  #99/1    fexit_noreturns/noreturns:OK
  #99      fexit_noreturns:OK
  Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 .../selftests/bpf/prog_tests/fexit_noreturns.c    |  9 +++++++++
 .../testing/selftests/bpf/progs/fexit_noreturns.c | 15 +++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fexit_noreturns.c
 create mode 100644 tools/testing/selftests/bpf/progs/fexit_noreturns.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_noreturns.c b/tools/testing/selftests/bpf/prog_tests/fexit_noreturns.c
new file mode 100644
index 000000000000..568d3aa48a78
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_noreturns.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "fexit_noreturns.skel.h"
+
+void test_fexit_noreturns(void)
+{
+	RUN_TESTS(fexit_noreturns);
+}
diff --git a/tools/testing/selftests/bpf/progs/fexit_noreturns.c b/tools/testing/selftests/bpf/progs/fexit_noreturns.c
new file mode 100644
index 000000000000..a8d25b21f7c2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/fexit_noreturns.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("fexit/do_exit")
+__failure __msg("Attaching fexit to __noreturn functions is rejected.")
+int BPF_PROG(noreturns)
+{
+	return 0;
+}
-- 
2.43.5

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Alexei Starovoitov]

On Mon, Feb 24, 2025 at 3:46 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> +       } else if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
> +                  btf_id_set_contains(&fexit_deny, btf_id)) {
> +               verbose(env, "Attaching fexit to __noreturn functions is rejected.\n");
> +               return -EINVAL;

Just realized that this needs to include
prog->expected_attach_type == BPF_MODIFY_RETURN
since it's doing __bpf_tramp_enter() too.

Also the list must only contain existing functions.
Otherwise there are plenty of build warns:
  BTFIDS  vmlinux
WARN: resolve_btfids: unresolved symbol xen_start_kernel
WARN: resolve_btfids: unresolved symbol xen_cpu_bringup_again
WARN: resolve_btfids: unresolved symbol usercopy_abort
WARN: resolve_btfids: unresolved symbol snp_abort
WARN: resolve_btfids: unresolved symbol sev_es_terminate
WARN: resolve_btfids: unresolved symbol rust_helper_BUG
...

pw-bot: cr

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Yafang Shao]

On Tue, Feb 25, 2025 at 1:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 24, 2025 at 3:46 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > +       } else if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
> > +                  btf_id_set_contains(&fexit_deny, btf_id)) {
> > +               verbose(env, "Attaching fexit to __noreturn functions is rejected.\n");
> > +               return -EINVAL;
>
> Just realized that this needs to include
> prog->expected_attach_type == BPF_MODIFY_RETURN
> since it's doing __bpf_tramp_enter() too.

I will add it.

>
> Also the list must only contain existing functions.
> Otherwise there are plenty of build warns:
>   BTFIDS  vmlinux
> WARN: resolve_btfids: unresolved symbol xen_start_kernel
> WARN: resolve_btfids: unresolved symbol xen_cpu_bringup_again
> WARN: resolve_btfids: unresolved symbol usercopy_abort
> WARN: resolve_btfids: unresolved symbol snp_abort
> WARN: resolve_btfids: unresolved symbol sev_es_terminate
> WARN: resolve_btfids: unresolved symbol rust_helper_BUG
> ...

I missed these warnings.
It looks like we need to add "#ifdef XXXX" to each function.
Alternatively, could we just compare the function name with
prog->aux->attach_func_name instead?

--
Regards
Yafang

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Josh Poimboeuf]

On Mon, Feb 24, 2025 at 07:46:05PM +0800, Yafang Shao wrote:
> +BTF_SET_START(fexit_deny)
> +#define NORETURN(fn) BTF_ID(func, fn)
> +NORETURN(__fortify_panic)
> +NORETURN(__ia32_sys_exit)
> +NORETURN(__ia32_sys_exit_group)

Why not just sync with the objtool noreturns.h?  Now we have two
hard-coded lists instead of one.  These are guaranteed to diverge.

-- 
Josh

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Alexei Starovoitov]

On Mon, Feb 24, 2025 at 11:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Feb 25, 2025 at 1:30 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 3:46 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > +       } else if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
> > > +                  btf_id_set_contains(&fexit_deny, btf_id)) {
> > > +               verbose(env, "Attaching fexit to __noreturn functions is rejected.\n");
> > > +               return -EINVAL;
> >
> > Just realized that this needs to include
> > prog->expected_attach_type == BPF_MODIFY_RETURN
> > since it's doing __bpf_tramp_enter() too.
>
> I will add it.
>
> >
> > Also the list must only contain existing functions.
> > Otherwise there are plenty of build warns:
> >   BTFIDS  vmlinux
> > WARN: resolve_btfids: unresolved symbol xen_start_kernel
> > WARN: resolve_btfids: unresolved symbol xen_cpu_bringup_again
> > WARN: resolve_btfids: unresolved symbol usercopy_abort
> > WARN: resolve_btfids: unresolved symbol snp_abort
> > WARN: resolve_btfids: unresolved symbol sev_es_terminate
> > WARN: resolve_btfids: unresolved symbol rust_helper_BUG
> > ...
>
> I missed these warnings.
> It looks like we need to add "#ifdef XXXX" to each function.
> Alternatively, could we just compare the function name with
> prog->aux->attach_func_name instead?

Strings are much less efficient than btf_ids.
Especially comparing across many strings.
To minimize ifdef-s lets remove all functions that bpf cannot
attach anyway (that are not in available_filter_functions).
Then drop all that call panic/BUG equivalent,
since refcnt on trampoline is irrelevant at that point.
That will remove even more functions.
At the end the list will be short with few ifdef-s.
This is a temporary workaround anyway, so let's not get too creative.

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Yafang Shao]

On Wed, Feb 26, 2025 at 1:52 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 24, 2025 at 07:46:05PM +0800, Yafang Shao wrote:
> > +BTF_SET_START(fexit_deny)
> > +#define NORETURN(fn) BTF_ID(func, fn)
> > +NORETURN(__fortify_panic)
> > +NORETURN(__ia32_sys_exit)
> > +NORETURN(__ia32_sys_exit_group)
>
> Why not just sync with the objtool noreturns.h?  Now we have two
> hard-coded lists instead of one.  These are guaranteed to diverge.

For details, refer to Alexei's response here:
https://lore.kernel.org/bpf/CAADnVQ+zLZKyrNGnGQDThasdS6cvM-FheN5Ttz23pF5ttbGasw@mail.gmail.com/.

We cannot simply reuse the objtool noreturns.h file because BTF is
limited to handling only existing functions.

--
Regards

Yafang

Re: [PATCH v3 1/2] bpf: Reject attaching fexit to functions annotated with __noreturn

[Yafang Shao]

On Wed, Feb 26, 2025 at 2:39 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 24, 2025 at 11:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 1:30 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 3:46 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > +       } else if (prog->expected_attach_type == BPF_TRACE_FEXIT &&
> > > > +                  btf_id_set_contains(&fexit_deny, btf_id)) {
> > > > +               verbose(env, "Attaching fexit to __noreturn functions is rejected.\n");
> > > > +               return -EINVAL;
> > >
> > > Just realized that this needs to include
> > > prog->expected_attach_type == BPF_MODIFY_RETURN
> > > since it's doing __bpf_tramp_enter() too.
> >
> > I will add it.
> >
> > >
> > > Also the list must only contain existing functions.
> > > Otherwise there are plenty of build warns:
> > >   BTFIDS  vmlinux
> > > WARN: resolve_btfids: unresolved symbol xen_start_kernel
> > > WARN: resolve_btfids: unresolved symbol xen_cpu_bringup_again
> > > WARN: resolve_btfids: unresolved symbol usercopy_abort
> > > WARN: resolve_btfids: unresolved symbol snp_abort
> > > WARN: resolve_btfids: unresolved symbol sev_es_terminate
> > > WARN: resolve_btfids: unresolved symbol rust_helper_BUG
> > > ...
> >
> > I missed these warnings.
> > It looks like we need to add "#ifdef XXXX" to each function.
> > Alternatively, could we just compare the function name with
> > prog->aux->attach_func_name instead?
>
> Strings are much less efficient than btf_ids.
> Especially comparing across many strings.
> To minimize ifdef-s lets remove all functions that bpf cannot
> attach anyway (that are not in available_filter_functions).
> Then drop all that call panic/BUG equivalent,
> since refcnt on trampoline is irrelevant at that point.
> That will remove even more functions.
> At the end the list will be short with few ifdef-s.
> This is a temporary workaround anyway, so let's not get too creative.

Understood

-- 
Regards
Yafang