From: "Emil Tsalapatis" <emil@etsalapatis.com>
To: "Eduard Zingerman" <eddyz87@gmail.com>, <bpf@vger.kernel.org>
Cc: <andrii@kernel.org>, <ast@kernel.org>, <daniel@iogearbox.net>,
<martin.lau@kernel.org>, <memxor@gmail.com>, <song@kernel.org>,
<yonghong.song@linux.dev>
Subject: Re: [PATCH bpf-next v4 1/4] bpf: Factor out program return value calculation
Date: Thu, 26 Feb 2026 16:17:01 -0500 [thread overview]
Message-ID: <DGP7F6QOXDK0.1LUCS3MPFYJOL@etsalapatis.com> (raw)
In-Reply-To: <7d080cc5fe67d32fef566a13cee0cd933b13d6f8.camel@gmail.com>
On Wed Feb 25, 2026 at 6:31 PM EST, Eduard Zingerman wrote:
> On Tue, 2026-02-24 at 22:33 -0500, Emil Tsalapatis wrote:
>> Factor the return value range calculation logic in check_return_code
>> out of the function in preparation for separating the return value
>> validation logic for BPF_EXIT and bpf_throw().
>>
>> Signed-off-by: Emil Tsalapatis <emil@etsalapatis.com>
>> ---
>
> I like this refactoring, thank you! The logic seem to be preserved.
> A few nits below.
>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
>> kernel/bpf/verifier.c | 205 +++++++++++++++++++++++-------------------
>> 1 file changed, 114 insertions(+), 91 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index edf5342b982f..96ec27a36b32 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -17837,82 +17837,14 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn
>> *insn)
>> return 0;
>> }
>>
>> -static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name)
>> +
>> +static int return_retval_range(struct bpf_verifier_env *env, struct bpf_retval_range *range,
>> + bool *return_32bit)
>
> Nit: maybe make this function bool?
> It does not seem to return any errors.
Annoyingly, there is a _single_ -EOPNOTSUPP for TRACING programs that do
not have one of 6 attach types (FENTRY/FEXIT/FSESSION/RAW_TP/MODIFY_RETURN/TRACE_ITER).
IIUC This is actually an exhaustive list of all attach types for a TRACING program, so the
-EOPNOTSUPP is redundant - we check the attach type is valid at attach/link creation time
with bpf_prog_attach_check_attach_type. We can have any invalid attach types for TRACING
progs fall through to the default range and turn the whole function into a bool.
>
>> {
>> - const char *exit_ctx = "At program exit";
>> - struct tnum enforce_attach_type_range = tnum_unknown;
>> - const struct bpf_prog *prog = env->prog;
>> - struct bpf_reg_state *reg = reg_state(env, regno);
>> - struct bpf_retval_range range = retval_range(0, 1);
>> enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
>> - int err;
>> - struct bpf_func_state *frame = env->cur_state->frame[0];
>> - const bool is_subprog = frame->subprogno;
>> - bool return_32bit = false;
>> - const struct btf_type *reg_type, *ret_type = NULL;
>> -
>> - /* LSM and struct_ops func-ptr's return type could be "void" */
>> - if (!is_subprog || frame->in_exception_callback_fn) {
>> - switch (prog_type) {
>> - case BPF_PROG_TYPE_LSM:
>> - if (prog->expected_attach_type == BPF_LSM_CGROUP)
>> - /* See below, can be 0 or 0-1 depending on hook. */
>> - break;
>> - if (!prog->aux->attach_func_proto->type)
>> - return 0;
>> - break;
>> - case BPF_PROG_TYPE_STRUCT_OPS:
>> - if (!prog->aux->attach_func_proto->type)
>> - return 0;
>> -
>> - if (frame->in_exception_callback_fn)
>> - break;
>> -
>> - /* Allow a struct_ops program to return a referenced kptr if it
>> - * matches the operator's return type and is in its unmodified
>> - * form. A scalar zero (i.e., a null pointer) is also allowed.
>> - */
>> - reg_type = reg->btf ? btf_type_by_id(reg->btf, reg->btf_id) : NULL;
>> - ret_type = btf_type_resolve_ptr(prog->aux->attach_btf,
>> - prog->aux->attach_func_proto->type,
>> - NULL);
>> - if (ret_type && ret_type == reg_type && reg->ref_obj_id)
>> - return __check_ptr_off_reg(env, reg, regno, false);
>> - break;
>> - default:
>> - break;
>> - }
>> - }
>>
>> - /* eBPF calling convention is such that R0 is used
>> - * to return the value from eBPF program.
>> - * Make sure that it's readable at this time
>> - * of bpf_exit, which means that program wrote
>> - * something into it earlier
>> - */
>> - err = check_reg_arg(env, regno, SRC_OP);
>> - if (err)
>> - return err;
>> -
>> - if (is_pointer_value(env, regno)) {
>> - verbose(env, "R%d leaks addr as return value\n", regno);
>> - return -EACCES;
>> - }
>> -
>> - if (frame->in_async_callback_fn) {
>> - exit_ctx = "At async callback return";
>> - range = frame->callback_ret_range;
>> - goto enforce_retval;
>> - }
>> -
>> - if (is_subprog && !frame->in_exception_callback_fn) {
>> - if (reg->type != SCALAR_VALUE) {
>> - verbose(env, "At subprogram exit the register R%d is not a scalar value
>> (%s)\n",
>> - regno, reg_type_str(env, reg->type));
>> - return -EINVAL;
>> - }
>> - return 0;
>> - }
>> + /* Default return value range. */
>> + *range = retval_range(0, 1);
>>
>> switch (prog_type) {
>> case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
>> @@ -17925,16 +17857,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const
>> char
>> env->prog->expected_attach_type == BPF_CGROUP_INET4_GETSOCKNAME ||
>> env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME ||
>> env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETSOCKNAME)
>> - range = retval_range(1, 1);
>> + *range = retval_range(1, 1);
>> if (env->prog->expected_attach_type == BPF_CGROUP_INET4_BIND ||
>> env->prog->expected_attach_type == BPF_CGROUP_INET6_BIND)
>> - range = retval_range(0, 3);
>> + *range = retval_range(0, 3);
>> break;
>> case BPF_PROG_TYPE_CGROUP_SKB:
>> - if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) {
>> - range = retval_range(0, 3);
>> - enforce_attach_type_range = tnum_range(2, 3);
>> - }
>> + if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS)
>> + *range = retval_range(0, 3);
>> break;
>> case BPF_PROG_TYPE_CGROUP_SOCK:
>> case BPF_PROG_TYPE_SOCK_OPS:
>> @@ -17945,14 +17875,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const
>> char
>> case BPF_PROG_TYPE_RAW_TRACEPOINT:
>> if (!env->prog->aux->attach_btf_id)
>> return 0;
>> - range = retval_range(0, 0);
>> + *range = retval_range(0, 0);
>> break;
>> case BPF_PROG_TYPE_TRACING:
>> switch (env->prog->expected_attach_type) {
>> case BPF_TRACE_FENTRY:
>> case BPF_TRACE_FEXIT:
>> case BPF_TRACE_FSESSION:
>> - range = retval_range(0, 0);
>> + *range = retval_range(0, 0);
>> break;
>> case BPF_TRACE_RAW_TP:
>> case BPF_MODIFY_RETURN:
>> @@ -17967,40 +17897,37 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const
>> char
>> switch (env->prog->expected_attach_type) {
>> case BPF_TRACE_KPROBE_SESSION:
>> case BPF_TRACE_UPROBE_SESSION:
>> - range = retval_range(0, 1);
>> break;
>> default:
>> return 0;
>> }
>> break;
>> case BPF_PROG_TYPE_SK_LOOKUP:
>> - range = retval_range(SK_DROP, SK_PASS);
>> + *range = retval_range(SK_DROP, SK_PASS);
>> break;
>>
>> case BPF_PROG_TYPE_LSM:
>> if (env->prog->expected_attach_type != BPF_LSM_CGROUP) {
>> /* no range found, any return value is allowed */
>> - if (!get_func_retval_range(env->prog, &range))
>> + if (!get_func_retval_range(env->prog, range))
>> return 0;
>> /* no restricted range, any return value is allowed */
>> - if (range.minval == S32_MIN && range.maxval == S32_MAX)
>> + if (range->minval == S32_MIN && range->maxval == S32_MAX)
>
> Tangential to this refactoring. Looking at get_func_retval_range() it
> seems that S32_{MIN,MAX} special case can never happen.
> It defers to bpf_lsm_get_retval_range() which either does not set the
> range or it to [0, 1] or [-MAX_ERRNO, 0].
> Maybe remove it as a separate patch?
>
Ack, will do.
>> return 0;
>> - return_32bit = true;
>> + *return_32bit = true;
>
> Nit: maybe make this a special case in check_return_code(),
> as with enforce_attach_type_range?
> Or push 'return_32bit' to be a field in retval_range?
Ack, I think return_32bit as a field is cleaner conceptually. I will
keep the default value false to avoid churn on all retval_range()
call sites.
>
>> } else if (!env->prog->aux->attach_func_proto->type) {
>> /* Make sure programs that attach to void
>> * hooks don't try to modify return value.
>> */
>> - range = retval_range(1, 1);
>> + *range = retval_range(1, 1);
>> }
>> break;
>>
>> case BPF_PROG_TYPE_NETFILTER:
>> - range = retval_range(NF_DROP, NF_ACCEPT);
>> + *range = retval_range(NF_DROP, NF_ACCEPT);
>> break;
>> case BPF_PROG_TYPE_STRUCT_OPS:
>> - if (!ret_type)
>> - return 0;
>> - range = retval_range(0, 0);
>> + *range = retval_range(0, 0);
>> break;
>> case BPF_PROG_TYPE_EXT:
>> /* freplace program can return anything as its return value
>
> [...]
next prev parent reply other threads:[~2026-02-26 21:17 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-25 3:33 [PATCH bpf-next v4 0/4] bpf: Allow void return type for global subprogs Emil Tsalapatis
2026-02-25 3:33 ` [PATCH bpf-next v4 1/4] bpf: Factor out program return value calculation Emil Tsalapatis
2026-02-25 23:31 ` Eduard Zingerman
2026-02-26 21:17 ` Emil Tsalapatis [this message]
2026-02-25 3:33 ` [PATCH bpf-next v4 2/4] bpf: Separate bpf_throw() and bpf_exit() return value validation Emil Tsalapatis
2026-02-25 4:23 ` bot+bpf-ci
2026-02-26 1:48 ` Emil Tsalapatis
2026-02-26 4:56 ` Eduard Zingerman
2026-02-26 20:35 ` Emil Tsalapatis
2026-02-25 3:33 ` [PATCH bpf-next v4 3/4] bpf: Allow void global functions in the verifier Emil Tsalapatis
2026-02-25 3:33 ` [PATCH bpf-next v4 4/4] selftests: bpf: Add tests for void global subprogs Emil Tsalapatis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DGP7F6QOXDK0.1LUCS3MPFYJOL@etsalapatis.com \
--to=emil@etsalapatis.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=martin.lau@kernel.org \
--cc=memxor@gmail.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.