Re: [PATCH bpf-next v6 19/21] selftests/bpf: Add __set_global() and __ret_global() test tags

[Yonghong Song <yonghong.song@linux.dev>]

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