From: Yonghong Song <yonghong.song@linux.dev>
To: bot+bpf-ci@kernel.org, bpf@vger.kernel.org
Cc: ast@kernel.org, andrii@kernel.org, daniel@iogearbox.net,
eddyz87@gmail.com, kernel-team@fb.com, martin.lau@kernel.org,
mason@kernel.org, ihor.solodrai@linux.dev
Subject: Re: [PATCH bpf-next v6 19/21] selftests/bpf: Add __set_global() and __ret_global() test tags
Date: Sun, 27 Sep 2026 20:36:48 -0700 [thread overview]
Message-ID: <e2bef732-57ff-4000-9d89-36be91f27e07@linux.dev> (raw)
In-Reply-To: <a15ab7ae278b027898a8c42648927ac3787f041e33daec5237fc675f67a6eea1@mail.kernel.org>
On 9/27/26 1:24 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
>> index f3dbc3b59bff..9afa163fac5a 100644
>> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
>> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
>> index 25eeb1c1248b..65b55f7fa907 100644
>> --- a/tools/testing/selftests/bpf/test_loader.c
>> +++ b/tools/testing/selftests/bpf/test_loader.c
> [ ... ]
>
>> @@ -311,6 +326,142 @@ static int parse_caps(const char *str, __u64 *val, const char *name)
>> return 0;
>> }
>>
>> +static int access_global_var(struct bpf_object *obj, const char *name,
>> + __u64 *val, bool set)
>> +{
>> + __u32 off, sz, zero = 0;
>> + struct bpf_map *map;
>> + size_t vsz;
>> + void *buf;
>> + int err;
>> +
>> + err = find_global_var(obj, name, &map, &off, &sz);
>> + if (err)
>> + return err;
>> +
>> + vsz = bpf_map__value_size(map);
>> + buf = calloc(1, vsz);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + err = bpf_map__lookup_elem(map, &zero, sizeof(zero), buf, vsz, 0);
>> + if (err) {
>> + PRINT_FAIL("failed to read '%s': %d\n", name, err);
>> + goto out;
>> + }
>> + if (!set) {
>> + *val = sz == 4 ? *(__u32 *)(buf + off) : *(__u64 *)(buf + off);
>> + goto out;
>> + }
>> ^^^^
> Can __ret_global() match a negative expected value for a 4-byte variable?
>
> parse_global_var() uses strtoull(), which takes a leading '-' and negates
> it as a 64-bit unsigned value. So __ret_global(err, -22) stores
> 0xffffffffffffffea (64 bits).
>
> But access_global_var() reads the 4-byte variable zero-extended:
>
> *val = sz == 4 ? *(__u32 *)(buf + off) : *(__u64 *)(buf + off);
>
> and gets 0x00000000ffffffea (32 bits extended to 64). Then run_subtest()
> compares the two as __u64:
>
> if (v != subspec->ret_global_val) {
> PRINT_FAIL("Unexpected %s: 0x%llx != 0x%llx\n", ...);
>
> so the check always fails with "Unexpected err: 0xffffffea !=
> 0xffffffffffffffea", even though the program stored exactly -22.
>
> This matters in practice. Existing selftests keep negative errnos in
> 4-byte globals and check them from a hand-written driver, e.g.
> progs/test_bpf_nf.c has `int test_einval_reserved = 0;` checked in
> prog_tests/bpf_nf.c with `ASSERT_EQ(skel->bss->test_einval_reserved,
> -EINVAL, ...)`. The commit message says these tags are meant to replace
> that kind of driver. The neighbouring __retval() also supports negative
> values (parse_int() uses strtol() with a special case for INT_MIN), and 68
> programs under progs/ use __retval(-...).
>
> The write side does not have this problem. __set_global(err, -22)
> truncates to 0xffffffea and stores -22 correctly, so the same literal
> behaves differently in the two tags. 8-byte variables are fine because
> both sides are 64-bit.
>
> Should the expected value be truncated or sign-extended to the variable
> width (for example, compare only the low 32 bits when sz == 4, or
> sign-extend the value read back), or should out-of-range values be refused
> at parse time?
Right. Missed 4-byte value and also missed signedness. Will fix as commit
message promised.
>
> A related issue: __set_global() on a 4-byte variable silently drops any
> bits above 32:
>
>> + if (sz == 4)
>> + *(__u32 *)(buf + off) = *val;
> with no error. The tree allows it, but none of the tests in this series
> trigger it: they only use __u64 variables with non-negative values.
>
>> + else
>> + *(__u64 *)(buf + off) = *val;
>> + err = bpf_map__update_elem(map, &zero, sizeof(zero), buf, vsz, 0);
>> + if (err)
>> + PRINT_FAIL("failed to write '%s': %d\n", name, err);
>> +out:
>> + free(buf);
>> + return err;
>> +}
> [ ... ]
>
>> @@ -1532,6 +1720,13 @@ void run_subtest(struct test_loader *tester,
>> }
>> }
>>
>> + if (subspec->has_set_global) {
>> + __u64 v = subspec->set_global_val;
>> +
>> + if (access_global_var(tobj, subspec->set_global_var, &v, true))
>> + goto tobj_cleanup;
>> + }
>> +
>> err = do_prog_test_run(bpf_program__fd(tprog), &retval,
>> bpf_program__type(tprog) == BPF_PROG_TYPE_SYSCALL ? true : false,
>> spec->linear_sz);
>> @@ -1540,6 +1735,18 @@ void run_subtest(struct test_loader *tester,
>> goto tobj_cleanup;
>> }
>>
>> + if (subspec->has_ret_global) {
>> + __u64 v = 0;
>> +
>> + if (access_global_var(tobj, subspec->ret_global_var, &v, false))
>> + goto tobj_cleanup;
>> + if (v != subspec->ret_global_val) {
>> + PRINT_FAIL("Unexpected %s: 0x%llx != 0x%llx\n",
>> + subspec->ret_global_var, v, subspec->ret_global_val);
>> + goto tobj_cleanup;
>> + }
>> + }
>> ^^^^
> For a 4-byte variable, __ret_global() cannot match any expected value
> outside [0, UINT32_MAX], and that range includes every negative literal.
>
> parse_global_var() parses the value with strtoull(), so XSTR(-1) ("-1")
> becomes 0xffffffffffffffff. access_global_var() reads a 4-byte variable as
>
> *val = sz == 4 ? *(__u32 *)(buf + off) : *(__u64 *)(buf + off);
>
> which zero-extends it to 0x00000000ffffffff. The comparison here then
> always fails for `int cnt; ... __ret_global(cnt, -1)`, printing
> "Unexpected cnt: 0xffffffff != 0xffffffffffffffff" even when the program
> set cnt to -1.
>
> __set_global() does not have this problem: the same value truncated into a
> __u32 stores -1 correctly. So the two tags do not agree on what a 4-byte
> value is.
>
> Values too wide for the variable are also not rejected:
> __set_global(int_var, 0x100000000) quietly writes 0.
>
> The commit message says 4-byte variables are supported and cites veristat
> as the model, but veristat's set_global_var() checks signedness and range
> ("Variable %s value %lld is out of range [%lld; %lld]").
>
> Should the expected value be truncated or sign-adjusted to the variable
> width (for example, compare only the low 32 bits when sz == 4), or should
> out-of-range values be refused at parse time?
Yes, should do similar thing to veristat to have sz == 4 support
and signedness support.
>
> No test in the series hits this today; they all use the __u64 pads_ran
> with positive masks.
>
>> +
>> verify_stderr(bpf_program__fd(tprog), &subspec->stderr);
>>
>> if (subspec->stdout.cnt) {
>
> ---
> 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/36346422430
next prev parent reply other threads:[~2026-09-28 3:36 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-26 5:00 [PATCH bpf-next v6 00/21] bpf: Run exception cleanup landing pads when bpf_unwind() unwinds Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 01/21] bpf: Pack bpf_insn_aux_data flags into bit fields Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 02/21] bpf: Accept the compiler's exception cleanup table at program load Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 03/21] bpf: Add the bpf_unwind() and bpf_unwind_resume() kfuncs Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 04/21] bpf: Add lookups for exception cleanup resumes and landing pads Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 05/21] bpf: Prepare for an exception cleanup table before the CFG walk Yonghong Song
2026-09-26 5:16 ` sashiko-bot
2026-09-26 23:54 ` Yonghong Song
2026-09-27 20:39 ` bot+bpf-ci
2026-09-28 0:01 ` Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 06/21] bpf: Make exception landing pads reachable in the CFG Yonghong Song
2026-09-26 5:21 ` sashiko-bot
2026-09-27 0:02 ` Yonghong Song
2026-09-27 20:40 ` bot+bpf-ci
2026-09-28 0:12 ` Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 07/21] bpf: Resume a covered call at its landing pad Yonghong Song
2026-09-26 5:15 ` sashiko-bot
2026-09-26 8:21 ` Alexei Starovoitov
2026-09-27 0:04 ` Yonghong Song
2026-09-27 0:41 ` Yonghong Song
2026-09-27 20:40 ` bot+bpf-ci
2026-09-28 0:17 ` Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 08/21] bpf: Refuse a landing pad that does not resume Yonghong Song
2026-09-26 5:17 ` sashiko-bot
2026-09-27 3:06 ` Yonghong Song
2026-09-27 20:40 ` bot+bpf-ci
2026-09-28 0:29 ` Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 09/21] bpf: Refuse a private stack for a program with an exception cleanup table Yonghong Song
2026-09-26 5:00 ` [PATCH bpf-next v6 10/21] bpf: Dispatch cleanup pads by rewriting return addresses Yonghong Song
2026-09-27 20:40 ` bot+bpf-ci
2026-09-28 1:08 ` Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 11/21] bpf, x86: Dispatch exception cleanup pads at run time Yonghong Song
2026-09-26 5:15 ` sashiko-bot
2026-09-27 4:35 ` Yonghong Song
2026-09-27 20:39 ` bot+bpf-ci
2026-09-28 3:10 ` Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 12/21] bpf, arm64: " Yonghong Song
2026-09-26 5:14 ` sashiko-bot
2026-09-27 20:40 ` bot+bpf-ci
2026-09-26 5:01 ` [PATCH bpf-next v6 13/21] libbpf: Resolve the compiler's _Unwind_Resume to the kernel's kfunc Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 14/21] libbpf: Add cleanup_info to bpf_prog_load_opts Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 15/21] libbpf: Collect .bpf_cleanup records and pass them to the kernel Yonghong Song
2026-09-27 20:39 ` bot+bpf-ci
2026-09-28 3:28 ` Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 16/21] libbpf: Carry the exception cleanup table through the light skeleton Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 17/21] libbpf: Let the static linker carry .bpf_cleanup relocations Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 18/21] selftests/bpf: Add an end-to-end .bpf_cleanup exception test Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 19/21] selftests/bpf: Add __set_global() and __ret_global() test tags Yonghong Song
2026-09-26 5:18 ` sashiko-bot
2026-09-27 4:58 ` Yonghong Song
2026-09-27 20:24 ` bot+bpf-ci
2026-09-28 3:36 ` Yonghong Song [this message]
2026-09-26 5:01 ` [PATCH bpf-next v6 20/21] selftests/bpf: Cover the exception cleanup shapes the chain does not reach Yonghong Song
2026-09-27 20:40 ` bot+bpf-ci
2026-09-28 3:49 ` Yonghong Song
2026-09-26 5:01 ` [PATCH bpf-next v6 21/21] selftests/bpf: Load an exception cleanup program from a light skeleton Yonghong Song
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=e2bef732-57ff-4000-9d89-36be91f27e07@linux.dev \
--to=yonghong.song@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bot+bpf-ci@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=ihor.solodrai@linux.dev \
--cc=kernel-team@fb.com \
--cc=martin.lau@kernel.org \
--cc=mason@kernel.org \
/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