Re: [PATCH bpf-next 2/2] selftests/bpf: Cover refcount acquire node offsets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Logical double-free and verifier rejection in bpf_list_push_front error paths.
--

commit 822cb053457c04aa9becab9c713d49977dc74582
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>

selftests/bpf: Cover refcount acquire node offsets

This commit adds regression tests for bpf_refcount_acquire() when used
with graph-node-derived pointers. It includes both accepted cases with
normalized pointers and rejected cases with non-zero fixed offsets.

> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> index 61906f48025cc..7955a997316e0 100644
> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
[ ... ]
> +SEC("tc")
> +__description("refcount_acquire_list_pop_container: acquire normalized list pop")
> +__success __retval(0)
> +int refcount_acquire_list_pop_container(void *ctx)
> +{
> +	struct node_data *node, *base, *ref;
> +	struct bpf_list_node *list_node;
> +	long err;
> +
> +	node = bpf_obj_new(typeof(*node));
> +	if (!node)
> +		return -1;
> +
> +	bpf_spin_lock(&lock);
> +	err = bpf_list_push_front(&head, &node->l);
> +	if (err) {
> +		bpf_spin_unlock(&lock);
> +		bpf_obj_drop(node);

[Severity: Medium]
Does this cause a double-free and verifier rejection? 

Since bpf_list_push_front() consumes the referenced object unconditionally,
its reference ID is cleared upon return. The verifier treats the return value
as an unknown scalar and explores both branches of the error check.

Because calling bpf_obj_drop() requires an owning reference, dropping the
already-consumed node here might cause the __success test to fail loading
with a 'release kfunc expects referenced PTR_TO_BTF_ID' error.

> +		return -2;
> +	}
> +
> +	list_node = bpf_list_pop_front(&head);
> +	bpf_spin_unlock(&lock);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> index 7247a20c0a3bf..f6cb084af16aa 100644
> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
[ ... ]
> +SEC("?tc")
> +__failure __msg("R1 must have zero offset when passed to bpf_refcount_acquire")
> +long refcount_acquire_list_node_offset(void *ctx)
> +{
> +	struct node_refcounted *node, *base, *ref;
> +	struct bpf_list_node *list_node;
> +	long err;
> +
> +	node = bpf_obj_new(typeof(*node));
> +	if (!node)
> +		return 1;
> +
> +	bpf_spin_lock(&lock);
> +	err = bpf_list_push_front(&head, &node->list);
> +	if (err) {
> +		bpf_spin_unlock(&lock);
> +		bpf_obj_drop(node);

[Severity: Medium]
Similarly here in refcount_acquire_list_node_offset(), does dropping the
node after bpf_list_push_front() cause the same issue?

Depending on the verifier's branch evaluation order, this double-free logic
might cause the test to fail verification for the wrong reason, which would
break the expected __msg check.

> +		return 2;
> +	}
> +
> +	list_node = bpf_list_pop_front(&head);
> +	bpf_spin_unlock(&lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781852308.git.chenyy23@mails.tsinghua.edu.cn?part=2