Re: [PATCH bpf v3 2/3] bpf: Skip the progs of trampoline images that are being freed

[bot+bpf-ci@kernel.org]

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

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 2853e87797a7..7ef5c8efc882 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -3217,16 +3217,21 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog,
>  }
>
>  static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,
> +			   struct bpf_tramp_image *im,
>  			   struct bpf_tramp_node *node, int stack_size,
>  			   int run_ctx_off, bool save_ret,
>  			   void *image, void *rw_image)
>  {
>  	u8 *prog = *pprog;
> -	u8 *jmp_insn;
> +	u8 *jmp_insn, *skip;
>  	int ctx_cookie_off = offsetof(struct bpf_tramp_run_ctx, bpf_cookie);
>  	struct bpf_prog *p = node->link->prog;
>  	u64 cookie = node->cookie;
>
> +	/* nop, patched to skip this prog when the image is put */
> +	skip = image + (prog - (u8 *)rw_image);
> +	emit_nops(&prog, X86_PATCH_SIZE);
> +

The patch adds a 5-byte nop in front of every prog call sequence on x86
but keeps BPF_MAX_TRAMP_LINKS at 38. This adds 38 * 5 - 5 = 185 bytes to
the worst case image size, which was already close to PAGE_SIZE.

Example worst case: 19 fsession progs (38 link slots because each counts
twice) attached to a function with 7-9 argument registers, each calling
bpf_session_cookie() and bpf_get_func_ip() with a 64-bit link cookie.

With 7 args the image reaches about 4040-4043 bytes, above
PAGE_SIZE - BPF_INSN_SAFETY = 4032. With 10 args it reaches about
4106-4109 bytes. An attach that used to succeed now fails with -EFAULT
and a WARN_ON_ONCE() splat.

The size pass writes to a single PAGE_SIZE execmem buffer with only an
end-of-pass WARN_ON_ONCE check:

arch/x86/net/bpf_jit_comp.c:arch_bpf_trampoline_size() {
    image = execmem_alloc(EXECMEM_MODULE_DATA, PAGE_SIZE);
    ret = __arch_prepare_bpf_trampoline(&im, image, image + PAGE_SIZE, ...);

arch/x86/net/bpf_jit_comp.c:__arch_prepare_bpf_trampoline() {
    if (WARN_ON_ONCE(prog > (u8 *)rw_image_end - BPF_INSN_SAFETY)) {
        ret = -EFAULT;
        goto cleanup;
    }

With 10 args the size pass writes past the end of the execmem buffer
before the check is reached.

The commit message says the extra nops break the page budget only on
arm64 and loongarch. Should x86 get a lower BPF_MAX_TRAMP_LINKS too, or
should the x86 size pass bound-check as it emits?

[ ... ]

> @@ -3368,7 +3377,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>  	emit_mov_imm32(&prog, false, BPF_REG_0, 0);
>  	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
>  	for (i = 0; i < tl->nr_nodes; i++) {
> -		if (invoke_bpf_prog(m, &prog, tl->nodes[i], stack_size, run_ctx_off, true,
> +		if (invoke_bpf_prog(m, &prog, im, tl->nodes[i], stack_size, run_ctx_off, true,
>  				    image, rw_image))
>  			return -EINVAL;
>
>  		/* mod_ret prog stored return value into [rbp - 8]. Emit:
>  		 * if (*(u64 *)(rbp - 8) !=  0)
>  		 *	goto do_fexit;
>  		 */

invoke_bpf_mod_ret() now passes im to invoke_bpf_prog(), so every
fmod_ret prog gets a skip nop that bpf_tramp_image_put() patches to a
jump. The skip target is before the cmp/jne check. A skipped fmod_ret
prog therefore leaves the previous return value in place (0 from the
initial mov after xor eax,eax), and the original function is called.

Before this change only fexit progs were skipped in a put image, and
those only observe. fmod_ret progs always ran. Any trampoline update puts
the old image, including attaching or detaching an unrelated prog.

BPF LSM progs on int hooks become BPF_TRAMP_MODIFY_RETURN:

kernel/bpf/verifier.c:bpf_attach_type_to_tramp() {
    case BPF_LSM_MAC:
        if (!prog->aux->attach_func_proto->type)
            return BPF_TRAMP_FEXIT;
        else
            return BPF_TRAMP_MODIFY_RETURN;

Hooks such as bpf_lsm_file_open and bpf_lsm_bprm_check_security are in
sleepable_lsm_hooks. A possible sequence:

  task T in image I of bpf_lsm_file_open,
    sleeping in sleepable LSM prog S (returns 0)
                                     attach another prog to the hook
                                      -> new image, bpf_tramp_image_put(I)
                                      -> nop before enforcing prog P patched to jmp
  T returns from S
  jmp over P (P is still attached)
  cmp [rbp-8], 0 -> equal, call bpf_lsm_file_open() -> 0
  open allowed although P would have denied it

On a preemptible kernel the window exists even without sleepable progs:
anywhere between trampoline entry and P's nop.

The commit message says still-attached progs are skipped "like they
already skipped its fexit progs". For fmod_ret, though, skipping changes
the traced function's return value instead of just losing an event.

Is it intended that a concurrent attach or detach on the same hook can
bypass an attached LSM or fmod_ret prog?

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d4f732996b47..9c61803380c2 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1258,11 +1258,15 @@ struct btf_func_model {
>  #define BPF_TRAMP_F_INDIRECT		BIT(8)
>
>  /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
> - * bytes on x86.
> + * bytes on x86. The trampoline image has to fit in PAGE_SIZE.
>   */
>  enum {
> -#if defined(__s390x__)
> +#if defined(__s390x__) || defined(__powerpc64__)
>  	BPF_MAX_TRAMP_LINKS = 27,
> +#elif defined(__loongarch__)
> +	BPF_MAX_TRAMP_LINKS = 33,
> +#elif defined(__aarch64__)
> +	BPF_MAX_TRAMP_LINKS = 37,
>  #else
>  	BPF_MAX_TRAMP_LINKS = 38,
>  #endif

BPF_MAX_TRAMP_LINKS drops from 38 to 27 for every __powerpc64__ build
(and to 37 for every __aarch64__ build), whatever the page size. The
updated comment says the constraint is that the image has to fit in
PAGE_SIZE, and bpf_trampoline_update() does check the size against
PAGE_SIZE.

PPC_64K_PAGES is the default for PPC_BOOK3S_64, and 38 progs fit easily
in a 64K image. On common ppc64le configurations (and arm64 with 16K/64K
pages), attaching the 28th to 38th prog to one function now fails with
-E2BIG where it used to work.

The commit message's "already didn't on powerpc" only holds for 4K pages.

Could the limit depend on PAGE_SIZE instead, keeping 38 when the page is
larger than 4K?

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 9d69c066a817..0d7aa1b0fe30 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -611,6 +612,8 @@ static void __bpf_tramp_image_put_rcu_tasks(struct rcu_head *rcu)
>
>  static void bpf_tramp_image_put(struct bpf_tramp_image *im)
>  {
> +	int i, err;
> +
>  	/* The trampoline image that calls original function is using:
>  	 * rcu_read_lock_trace to protect sleepable bpf progs
>  	 * rcu_read_lock to protect normal bpf progs
> @@ -621,10 +624,17 @@ static void bpf_tramp_image_put(struct bpf_tramp_image *im)
>  	 *
>  	 * The trampoline is unreachable before bpf_tramp_image_put().
>  	 *
> -	 * First, patch the trampoline to avoid calling into fexit progs.
> -	 * The progs will be freed even if the original function is still
> -	 * executing or sleeping.
> -	 * In case of CONFIG_PREEMPT=y use call_rcu_tasks() to wait on
> +	 * First, patch the trampoline to avoid calling into progs. The progs
> +	 * will be freed even if a task is still in the trampoline, e.g.
> +	 * sleeping in the original function or in a sleepable prog.
> +	 */
> +	for (i = 0; i < im->nr_skips; i++) {
> +		err = bpf_arch_text_poke(im->skips[i].nop, BPF_MOD_NOP,
> +					 BPF_MOD_JUMP, NULL, im->skips[i].target);
> +		WARN_ON_ONCE(err);
> +	}

Now that every prog nop in the old image is patched, a task still in that
image also skips the fmod_ret progs that are still attached, not only
fentry/fexit progs.

Before this change only ip_after_call was patched. It sits after the
original function call, so fentry and fmod_ret progs always ran and only
fexit progs could be skipped.

The commit message justifies skipping attached progs "like they already
skipped its fexit progs". That comparison holds for observe-only
fentry/fexit progs, but fmod_ret progs decide the return value and
whether the original function runs.

BPF LSM progs on int-returning hooks are attached as
BPF_TRAMP_MODIFY_RETURN. Scenario:

1. Task T enters image I_old for bpf_lsm_file_open. It is either
   preempted or sleeping in an earlier sleepable lsm.s prog, e.g. blocked
   in bpf_copy_from_user(). Either way it has not yet reached the nop of
   enforcing LSM prog L.

2. Someone attaches or detaches any other prog on the same trampoline.
   bpf_trampoline_update() -> modify_fentry() -> bpf_tramp_image_put(I_old)
   turns all nops of I_old into jumps, including L's.

3. T resumes and jumps over L's call sequence. In x86 invoke_bpf_mod_ret()
   the result slot [rbp-8] was zeroed and nothing stores to it, so the
   following cmp/jne does not branch to do_fexit. The original
   bpf_lsm_file_open() stub is called and returns 0.

The hook fails open for T even though L is still attached and would have
denied access. The sequential pokes can also leave T running some
fmod_ret progs but not others. The sleepable case makes the window as
long as the earlier prog blocks.

Should fmod_ret progs that are still attached keep running, with only the
progs being removed skipped? Otherwise, is this fail-open behaviour for
BPF LSM and fmod_ret acceptable, and should the commit message and comment
say so?

> +
> +	/* In case of CONFIG_PREEMPT=y use call_rcu_tasks() to wait on
>  	 * first few asm instructions to execute and call into
>  	 * __bpf_tramp_enter->percpu_ref_get.
>  	 * Then use percpu_ref_kill to wait for the trampoline and the original

This isn't a bug, but the multi-line comment has the opening slash-star on
the same line as the comment text. BPF subsystem style in kernel/bpf/ has
the opening slash-star on its own line for new multi-line comments:

	/*
	 * In case of CONFIG_PREEMPT=y use call_rcu_tasks() to wait on
	 * first few asm instructions to execute and call into


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36034251810