Re: [PATCH bpf v2] bpf: Keep generic __uninit kfunc arguments live

[bot+bpf-ci@kernel.org]

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

> bpf: Keep generic __uninit kfunc arguments live
>
> Stack liveness models a kfunc's __uninit pointer argument as a pure write:
> whatever the buffer holds before the call is dead, and 2cb27158adb3 ("bpf:
> poison dead stack slots") poisons those slots at every checkpoint the path
> visits before the call. Argument validation disagrees. Only dynptr arguments
> honor __uninit. A generic fixed-size buffer goes through check_mem_reg(),
> whose read pass requires every byte readable. The two rules meet at the call
> and the poisoned buffer is rejected with "slot poisoned by dead code
> elimination", even when the program initialized it.
>
> Every kfunc with a generic __uninit output argument is affected,
> bpf_ksock_create() through err__uninit and sched_ext's scx_bpf_cid_topo()
> through its output struct. The rejection needs the buffer's slots to be
> allocated and dead at a checkpoint, for example on the second iteration of a
> loop around the call. Before e566701b9b0c ("bpf: Check fixed-size mem args
> of helpers and kfuncs the same way") kfunc arguments could read poisoned
> slots, so the call passed, but the slots stayed poisoned and the program's
> next read of the buffer was rejected instead.
>
> Restrict the liveness write-only exception to dynptr arguments, where
> validation accepts uninitialized output. A generic __uninit buffer stays
> live and is never poisoned, and validation keeps requiring it to be
> initialized. Add a selftest with a scalar output buffer and a checkpoint
> between its initialization and the call.
>
> v2: Rebased on bpf/master.

This isn't a bug, but could the 'v2: Rebased on bpf/master.' note move
below the --- line so it stays out of the git log?

> Fixes: 2cb27158adb3 ("bpf: poison dead stack slots")
> Cc: stable@vger.kernel.org # v7.1+

This isn't a bug, but since this targets bpf, would it be worth dropping
the Cc: stable trailer and instead noting the v7.1+ backport request
under the --- line, as bpf_devel_QA.rst asks?

> Signed-off-by: Tejun Heo <tj@kernel.org>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ddba53eaa3331..310a3c6339a98 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13588,7 +13588,8 @@ s64 bpf_kfunc_stack_access_bytes(struct bpf_verifier_env *env, struct bpf_insn *
>  	/* KF_ITER_NEW kfuncs initialize the iterator state at arg 0 */
>  	if (arg == 0 && meta.kfunc_flags & KF_ITER_NEW)
>  		return -size;
> -	if (is_kfunc_arg_uninit(btf, &args[arg]))
> +	/* only dynptr validation accepts an uninitialized __uninit argument */
> +	if (is_kfunc_arg_dynptr(btf, &args[arg]) && is_kfunc_arg_uninit(btf, &args[arg]))
>  		return -size;
>  	return size;
>  }

Amery Hung raised a design concern on v1 about whether restricting the
liveness exception to dynptrs is the right fix.

The reviewer suggests that the underlying issue is that generic argument
validation doesn't honor __uninit, and that generic __uninit arguments
should behave like helper output buffers: generate MEM_UNINIT | MEM_WRITE,
validate as write-only, and mark the written range initialized after the
kfunc call. With that approach, bpf_kfunc_stack_access_bytes() could
continue returning -size for generic __uninit arguments.

The current change instead makes their previous contents live and requires
callers to initialize an argument explicitly declared uninitialized.

Was this comment considered for v2?


---
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/34913753963