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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox