Re: [PATCH bpf v2 7/9] bpf: One may_goto time slice per prog invocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: 16-bit jump offset overflow in may_goto_expand_timed() under blinding
- [Medium] bpf: torn reads of cb->prog and cb->callback_fn in timer and wq callbacks
--

commit 382d62c2e4ba7bcffedca7ed4f3c49e710163db0
Author: Alexei Starovoitov <ast@kernel.org>

bpf: One may_goto time slice per prog invocation

This commit moves the timed may_goto count and timestamp from each subprog's
stack frame into per-CPU memory owned by the main program so that a single
250 ms time slice applies across an entire program invocation. It also saves
and restores the per-CPU timed may_goto state around asynchronous callbacks.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 5cbab55d84be0..66b9d1d9e961c 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -1803,6 +1803,50 @@ static int may_goto_expand(struct bpf_insn *insn_buf, int off, int stack_off,
>  	return cnt + tail_cnt;
>  }
>  
> +/*
> + * Timed may_goto. All may_goto of a prog use one count and one timestamp: per-cpu memory of the
> + * prog, set in the first insns of the main prog, and by the callers of async callbacks.
> + * @reg holds the address. It is a register that is dead at the may_goto, or R0 that is saved at
> + * @save_off in the stack.
> + */
> +static int may_goto_expand_timed(struct bpf_insn *insn_buf, int off,
> +				 struct bpf_timed_may_goto __percpu *tmg, int reg, int save_off)
> +{
> +	struct bpf_insn ld_tmg[] = { BPF_LD_IMM64(reg, (__force unsigned long)tmg) };
> +	int cnt = 0, jmp, tail_cnt = save_off ? 6 : 5;
> +
> +	if (save_off)
> +		insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, reg, save_off);
> +	insn_buf[cnt++] = ld_tmg[0];
> +	insn_buf[cnt++] = ld_tmg[1];
> +	insn_buf[cnt++] = BPF_MOV64_PERCPU_REG(reg, reg);
> +	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_AX, reg, 0);
> +	if (save_off) {
> +		insn_buf[cnt++] = BPF_JMP_IMM(BPF_JNE, BPF_REG_AX, 0, 2);
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, reg, BPF_REG_10, save_off);
> +		jmp = cnt++;
> +		insn_buf[jmp] = BPF_JMP32_A(off >= 0 ? off + tail_cnt : off - jmp);
> +	} else {
> +		jmp = cnt;
> +		off = off >= 0 ? off + tail_cnt : off - jmp;
> +		if (off == (s16)off) {
> +			insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, off);
> +		} else {
> +			insn_buf[cnt++] = BPF_JMP_IMM(BPF_JNE, BPF_REG_AX, 0, 1);
> +			insn_buf[cnt++] = BPF_JMP32_A(off >= 0 ? off : off - 1);
> +		}
> +	}

[Severity: Medium]
Can constant blinding cause the 16-bit jump offset in may_goto_expand_timed()
to overflow S16_MIN on backward branches?

When save_off is 0, may_goto_expand_timed() emits the 2-instruction
BPF_LD_IMM64(reg, tmg) into reg (one of BPF_REG_0..BPF_REG_9) before the
branch at jmp = 4. For a backward branch (orig_off < 0), off becomes
orig_off - 4, and if that fits in s16 (for example, orig_off in
[-32764, -32759]), it emits a 16-bit BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, off)
instead of BPF_JMP32_A.

Later, when JIT constant blinding is enabled,
bpf_jit_blind_constants() -> bpf_jit_blind_insn() expands the 2-instruction
BPF_LD_IMM64(reg, tmg) into 7 or 8 instructions:

kernel/bpf/core.c:bpf_jit_blind_insn() {
	...
	case BPF_LD | BPF_IMM | BPF_DW:
		*to++ = BPF_ALU64_IMM(BPF_MOV, BPF_REG_AX, imm_rnd ^ aux[1].imm);
		*to++ = BPF_ALU64_IMM(BPF_XOR, BPF_REG_AX, imm_rnd);
		*to++ = BPF_ALU64_IMM(BPF_LSH, BPF_REG_AX, 32);
		*to++ = BPF_ALU64_REG(BPF_MOV, aux[0].dst_reg, BPF_REG_AX);
		break;
	case 0: /* Part 2 of BPF_LD | BPF_IMM | BPF_DW. */
		*to++ = BPF_ALU32_IMM(BPF_MOV, BPF_REG_AX, imm_rnd ^ aux[0].imm);
		*to++ = BPF_ALU32_IMM(BPF_XOR, BPF_REG_AX, imm_rnd);
		if (emit_zext)
			*to++ = BPF_ZEXT_REG(BPF_REG_AX);
		*to++ = BPF_ALU64_REG(BPF_OR,  aux[0].dst_reg, BPF_REG_AX);
		break;
}

Because those 5 or 6 extra instructions are inserted between a backward jump
target and the 16-bit BPF_JEQ, bpf_adj_branches() -> bpf_adj_delta_to_off()
subtracts 5 or 6 from the 16-bit offset, underflowing S16_MIN and failing
blinding with -ERANGE. Since bpf_do_misc_fixups() sets prog->jit_required = 1,
the program cannot fall back to the interpreter and fails to load.

[ ... ]

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 58e357302850e..2b75f7ba07a56 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1222,6 +1222,8 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
>  {
>  	struct bpf_hrtimer *t = container_of(hrtimer, struct bpf_hrtimer, timer);
>  	struct bpf_map *map = READ_ONCE(t->cb.map);
> +	struct bpf_prog *prog = READ_ONCE(t->cb.prog);
> +	struct bpf_timed_may_goto saved;
>  	void *value = t->cb.value;
>  	bpf_callback_t callback_fn;
>  	void *key;
> @@ -1229,7 +1231,7 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
>  
>  	BTF_TYPE_EMIT(struct bpf_timer);
>  	callback_fn = rcu_dereference_check(t->cb.callback_fn, rcu_read_lock_bh_held());
> -	if (!callback_fn || !map)
> +	if (!callback_fn || !map || !prog)
>  		goto out;
>  
>  	/* bpf_timer_cb() runs in hrtimer_run_softirq. It doesn't migrate and
> @@ -1242,8 +1244,10 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
>  
>  	key = map_key_from_value(map, value, &idx);
>  
> +	bpf_timed_may_goto_save(prog, &saved);
>  	callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
>  	/* The verifier checked that return value is zero. */
> +	bpf_timed_may_goto_restore(prog, &saved);
>  
>  	this_cpu_write(hrtimer_running, NULL);
>  out:
> @@ -1254,6 +1258,8 @@ static void bpf_wq_work(struct work_struct *work)
>  {
>  	struct bpf_work *w = container_of(work, struct bpf_work, work);
>  	struct bpf_async_cb *cb = &w->cb;
> +	struct bpf_timed_may_goto saved;
> +	struct bpf_prog *prog;
>  	struct bpf_map *map;
>  	bpf_callback_t callback_fn;
>  	void *value = cb->value;
> @@ -1267,14 +1273,17 @@ static void bpf_wq_work(struct work_struct *work)
>  
>  	map = READ_ONCE(cb->map);
>  	callback_fn = READ_ONCE(cb->callback_fn);
> -	if (!callback_fn || !map)
> +	prog = READ_ONCE(cb->prog);
> +	if (!callback_fn || !map || !prog)
>  		goto out;
>  
>  	key = map_key_from_value(map, value, &idx);
>  
>  	migrate_disable();
>  
> +	bpf_timed_may_goto_save(prog, &saved);
>  	callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
> +	bpf_timed_may_goto_restore(prog, &saved);

[Severity: Medium]
What prevents bpf_timer_cb() and bpf_wq_work() from reading a mismatched pair
of cb->prog and cb->callback_fn when bpf_async_update_prog_callback() runs
concurrently?

In bpf_async_update_prog_callback(), cb->prog and cb->callback_fn are updated
with two separate lockless stores:

kernel/bpf/helpers.c:bpf_async_update_prog_callback() {
	...
	do {
		if (prog)
			prog = bpf_prog_inc_not_zero(prog);
		prev = xchg(&cb->prog, prog);
		rcu_assign_pointer(cb->callback_fn, callback_fn);
	...
}

When two programs share a map containing a bpf_timer or bpf_wq and update the
callback concurrently while the timer or workqueue fires, bpf_timer_cb() or
bpf_wq_work() can read prog from one program and callback_fn from the other.

If that happens, bpf_timed_may_goto_save() and bpf_timed_may_goto_restore()
will save, initialize, and restore the wrong program's per-CPU
timed_may_goto state while callback_fn accesses its own program's
timed_may_goto. If the callback's program still has count == 0 on this CPU
(because its main program has not run on this CPU yet or previously timed
out), its timed may_goto loops will terminate immediately on the first
iteration, or if it interrupted its own main program on the same CPU, the
interrupted invocation's count and timestamp will not be saved and restored.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031215543.682284-1-alexei.starovoitov@gmail.com?part=7