Re: [PATCH bpf-next v1 6/6] selftests/bpf: Cover callback-frame map key lifetime

[bot+bpf-ci]

> selftests/bpf: Cover callback-frame map key lifetime
>
> Parking the key in callback_ctx and dereferencing it after the iteration
> is rejected for array and percpu-array maps, whose key lives in
> bpf_for_each_array_elem()'s frame.
>
> Two cases must keep verifying, and are the reason the declaration is not
> in the shared map_set_for_each_callback_args(): the same shape over a
> hash map, whose key points into the element, and parking the element
> value, which lives until map teardown.

This isn't a bug, but would it read more precisely to split these - the
hash-map case as the reason the declaration is per-map-ops, and the
parked-value case as a guard that only the key was declared frame-scoped?

Looking at 6cd471d244f2, the placement decision is driven by the first case
(hash_park_map_key), since that's what would break if mark_frame_scoped_arg
for BPF_REG_2 moved into the shared map_set_for_each_callback_args. The
second case (array_park_map_value) parks R3 (PTR_TO_MAP_VALUE), which no
placement of a BPF_REG_2 declaration can affect.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> index 1fbcc5228306..2e0c56888953 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c

[ ... ]

> @@ -800,4 +814,81 @@ __naked void check_add_const_regsafe_off(void)
>  	: __clobber_common);
>  }
>
> +struct key_ctx {
> +	__u32 *key;
> +};
> +
> +static long park_key_cb(struct bpf_map *map, __u32 *key, __u64 *value,
> +			void *context)
> +{
> +	struct key_ctx *c = context;
> +
> +	c->key = key;
> +	return 0;
> +}
> +
> +/* bpf_for_each_array_elem() passes a key from its own stack frame. */
> +SEC("?raw_tp")
> +__failure __msg("invalid mem access 'scalar'")
> +int array_park_map_key(void *ctx)
> +{
> +	struct key_ctx c = {};
> +
> +	bpf_for_each_map_elem(&map, park_key_cb, &c, 0);
> +	if (c.key)
> +		return *c.key;
> +	return 0;
> +}
> +
> +SEC("?raw_tp")
> +__failure __msg("invalid mem access 'scalar'")
> +int percpu_array_park_map_key(void *ctx)
> +{
> +	struct key_ctx c = {};
> +
> +	bpf_for_each_map_elem(&percpu_map, park_key_cb, &c, 0);
> +	if (c.key)
> +		return *c.key;
> +	return 0;
> +}
> +
> +/* A hash key points into the element, which outlives the callback. */
> +SEC("?raw_tp")
> +__success
> +int hash_park_map_key(void *ctx)
> +{
> +	struct key_ctx c = {};
> +
> +	bpf_for_each_map_elem(&hash_map, park_key_cb, &c, 0);
> +	if (c.key)
> +		return *c.key;
> +	return 0;
> +}
> +
> +struct value_ctx {
> +	__u64 *value;
> +};
> +
> +static long park_value_cb(struct bpf_map *map, __u32 *key, __u64 *value,
> +			  void *context)
> +{
> +	struct value_ctx *c = context;
> +
> +	c->value = value;
> +	return 0;
> +}
> +
> +/* Only the key is frame-scoped; the element lives until map teardown. */
> +SEC("?raw_tp")
> +__success
> +int array_park_map_value(void *ctx)
> +{
> +	struct value_ctx c = {};
> +
> +	bpf_for_each_map_elem(&map, park_value_cb, &c, 0);
> +	if (c.value)
> +		return *c.value;
> +	return 0;
> +}
> +
>  char _license[] SEC("license") = "GPL";


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