404ad75a36
This patch modifies bpf_rbtree_remove to account for possible failure
due to the input rb_node already not being in any collection.
The function can now return NULL, and does when the aforementioned
scenario occurs. As before, on successful removal an owning reference to
the removed node is returned.
Adding KF_RET_NULL to bpf_rbtree_remove's kfunc flags - now KF_RET_NULL |
KF_ACQUIRE - provides the desired verifier semantics:
* retval must be checked for NULL before use
* if NULL, retval's ref_obj_id is released
* retval is a "maybe acquired" owning ref, not a non-owning ref,
so it will live past end of critical section (bpf_spin_unlock), and
thus can be checked for NULL after the end of the CS
BPF programs must add checks
============================
This does change bpf_rbtree_remove's verifier behavior. BPF program
writers will need to add NULL checks to their programs, but the
resulting UX looks natural:
bpf_spin_lock(&glock);
n = bpf_rbtree_first(&ghead);
if (!n) { /* ... */}
res = bpf_rbtree_remove(&ghead, &n->node);
bpf_spin_unlock(&glock);
if (!res) /* Newly-added check after this patch */
return 1;
n = container_of(res, /* ... */);
/* Do something else with n */
bpf_obj_drop(n);
return 0;
The "if (!res)" check above is the only addition necessary for the above
program to pass verification after this patch.
bpf_rbtree_remove no longer clobbers non-owning refs
====================================================
An issue arises when bpf_rbtree_remove fails, though. Consider this
example:
struct node_data {
long key;
struct bpf_list_node l;
struct bpf_rb_node r;
struct bpf_refcount ref;
};
long failed_sum;
void bpf_prog()
{
struct node_data *n = bpf_obj_new(/* ... */);
struct bpf_rb_node *res;
n->key = 10;
bpf_spin_lock(&glock);
bpf_list_push_back(&some_list, &n->l); /* n is now a non-owning ref */
res = bpf_rbtree_remove(&some_tree, &n->r, /* ... */);
if (!res)
failed_sum += n->key; /* not possible */
bpf_spin_unlock(&glock);
/* if (res) { do something useful and drop } ... */
}
The bpf_rbtree_remove in this example will always fail. Similarly to
bpf_spin_unlock, bpf_rbtree_remove is a non-owning reference
invalidation point. The verifier clobbers all non-owning refs after a
bpf_rbtree_remove call, so the "failed_sum += n->key" line will fail
verification, and in fact there's no good way to get information about
the node which failed to add after the invalidation. This patch removes
non-owning reference invalidation from bpf_rbtree_remove to allow the
above usecase to pass verification. The logic for why this is now
possible is as follows:
Before this series, bpf_rbtree_add couldn't fail and thus assumed that
its input, a non-owning reference, was in the tree. But it's easy to
construct an example where two non-owning references pointing to the same
underlying memory are acquired and passed to rbtree_remove one after
another (see rbtree_api_release_aliasing in
selftests/bpf/progs/rbtree_fail.c).
So it was necessary to clobber non-owning refs to prevent this
case and, more generally, to enforce "non-owning ref is definitely
in some collection" invariant. This series removes that invariant and
the failure / runtime checking added in this patch provide a clean way
to deal with the aliasing issue - just fail to remove.
Because the aliasing issue prevented by clobbering non-owning refs is no
longer an issue, this patch removes the invalidate_non_owning_refs
call from verifier handling of bpf_rbtree_remove. Note that
bpf_spin_unlock - the other caller of invalidate_non_owning_refs -
clobbers non-owning refs for a different reason, so its clobbering
behavior remains unchanged.
No BPF program changes are necessary for programs to remain valid as a
result of this clobbering change. A valid program before this patch
passed verification with its non-owning refs having shorter (or equal)
lifetimes due to more aggressive clobbering.
Also, update existing tests to check bpf_rbtree_remove retval for NULL
where necessary, and move rbtree_api_release_aliasing from
progs/rbtree_fail.c to progs/rbtree.c since it's now expected to pass
verification.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Link: https://lore.kernel.org/r/20230415201811.343116-8-davemarchevsky@fb.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
303 lines
6.7 KiB
C
303 lines
6.7 KiB
C
// SPDX-License-Identifier: GPL-2.0
|
|
#include <vmlinux.h>
|
|
#include <bpf/bpf_tracing.h>
|
|
#include <bpf/bpf_helpers.h>
|
|
#include <bpf/bpf_core_read.h>
|
|
#include "bpf_experimental.h"
|
|
#include "bpf_misc.h"
|
|
|
|
struct node_data {
|
|
long key;
|
|
long data;
|
|
struct bpf_rb_node node;
|
|
};
|
|
|
|
#define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8)))
|
|
private(A) struct bpf_spin_lock glock;
|
|
private(A) struct bpf_rb_root groot __contains(node_data, node);
|
|
private(A) struct bpf_rb_root groot2 __contains(node_data, node);
|
|
|
|
static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
|
|
{
|
|
struct node_data *node_a;
|
|
struct node_data *node_b;
|
|
|
|
node_a = container_of(a, struct node_data, node);
|
|
node_b = container_of(b, struct node_data, node);
|
|
|
|
return node_a->key < node_b->key;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
|
|
long rbtree_api_nolock_add(void *ctx)
|
|
{
|
|
struct node_data *n;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
bpf_rbtree_add(&groot, &n->node, less);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
|
|
long rbtree_api_nolock_remove(void *ctx)
|
|
{
|
|
struct node_data *n;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
bpf_spin_lock(&glock);
|
|
bpf_rbtree_add(&groot, &n->node, less);
|
|
bpf_spin_unlock(&glock);
|
|
|
|
bpf_rbtree_remove(&groot, &n->node);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
|
|
long rbtree_api_nolock_first(void *ctx)
|
|
{
|
|
bpf_rbtree_first(&groot);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("rbtree_remove node input must be non-owning ref")
|
|
long rbtree_api_remove_unadded_node(void *ctx)
|
|
{
|
|
struct node_data *n, *m;
|
|
struct bpf_rb_node *res;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
m = bpf_obj_new(typeof(*m));
|
|
if (!m) {
|
|
bpf_obj_drop(n);
|
|
return 1;
|
|
}
|
|
|
|
bpf_spin_lock(&glock);
|
|
bpf_rbtree_add(&groot, &n->node, less);
|
|
|
|
/* This remove should pass verifier */
|
|
res = bpf_rbtree_remove(&groot, &n->node);
|
|
n = container_of(res, struct node_data, node);
|
|
|
|
/* This remove shouldn't, m isn't in an rbtree */
|
|
res = bpf_rbtree_remove(&groot, &m->node);
|
|
m = container_of(res, struct node_data, node);
|
|
bpf_spin_unlock(&glock);
|
|
|
|
if (n)
|
|
bpf_obj_drop(n);
|
|
if (m)
|
|
bpf_obj_drop(m);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("Unreleased reference id=3 alloc_insn=10")
|
|
long rbtree_api_remove_no_drop(void *ctx)
|
|
{
|
|
struct bpf_rb_node *res;
|
|
struct node_data *n;
|
|
|
|
bpf_spin_lock(&glock);
|
|
res = bpf_rbtree_first(&groot);
|
|
if (!res)
|
|
goto unlock_err;
|
|
|
|
res = bpf_rbtree_remove(&groot, res);
|
|
|
|
if (res) {
|
|
n = container_of(res, struct node_data, node);
|
|
__sink(n);
|
|
}
|
|
bpf_spin_unlock(&glock);
|
|
|
|
/* if (res) { bpf_obj_drop(n); } is missing here */
|
|
return 0;
|
|
|
|
unlock_err:
|
|
bpf_spin_unlock(&glock);
|
|
return 1;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("arg#1 expected pointer to allocated object")
|
|
long rbtree_api_add_to_multiple_trees(void *ctx)
|
|
{
|
|
struct node_data *n;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
bpf_spin_lock(&glock);
|
|
bpf_rbtree_add(&groot, &n->node, less);
|
|
|
|
/* This add should fail since n already in groot's tree */
|
|
bpf_rbtree_add(&groot2, &n->node, less);
|
|
bpf_spin_unlock(&glock);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("dereference of modified ptr_or_null_ ptr R2 off=16 disallowed")
|
|
long rbtree_api_use_unchecked_remove_retval(void *ctx)
|
|
{
|
|
struct bpf_rb_node *res;
|
|
|
|
bpf_spin_lock(&glock);
|
|
|
|
res = bpf_rbtree_first(&groot);
|
|
if (!res)
|
|
goto err_out;
|
|
res = bpf_rbtree_remove(&groot, res);
|
|
|
|
bpf_spin_unlock(&glock);
|
|
|
|
bpf_spin_lock(&glock);
|
|
/* Must check res for NULL before using in rbtree_add below */
|
|
bpf_rbtree_add(&groot, res, less);
|
|
bpf_spin_unlock(&glock);
|
|
return 0;
|
|
|
|
err_out:
|
|
bpf_spin_unlock(&glock);
|
|
return 1;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("rbtree_remove node input must be non-owning ref")
|
|
long rbtree_api_add_release_unlock_escape(void *ctx)
|
|
{
|
|
struct node_data *n;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
bpf_spin_lock(&glock);
|
|
bpf_rbtree_add(&groot, &n->node, less);
|
|
bpf_spin_unlock(&glock);
|
|
|
|
bpf_spin_lock(&glock);
|
|
/* After add() in previous critical section, n should be
|
|
* release_on_unlock and released after previous spin_unlock,
|
|
* so should not be possible to use it here
|
|
*/
|
|
bpf_rbtree_remove(&groot, &n->node);
|
|
bpf_spin_unlock(&glock);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("rbtree_remove node input must be non-owning ref")
|
|
long rbtree_api_first_release_unlock_escape(void *ctx)
|
|
{
|
|
struct bpf_rb_node *res;
|
|
struct node_data *n;
|
|
|
|
bpf_spin_lock(&glock);
|
|
res = bpf_rbtree_first(&groot);
|
|
if (!res) {
|
|
bpf_spin_unlock(&glock);
|
|
return 1;
|
|
}
|
|
n = container_of(res, struct node_data, node);
|
|
bpf_spin_unlock(&glock);
|
|
|
|
bpf_spin_lock(&glock);
|
|
/* After first() in previous critical section, n should be
|
|
* release_on_unlock and released after previous spin_unlock,
|
|
* so should not be possible to use it here
|
|
*/
|
|
bpf_rbtree_remove(&groot, &n->node);
|
|
bpf_spin_unlock(&glock);
|
|
return 0;
|
|
}
|
|
|
|
static bool less__bad_fn_call_add(struct bpf_rb_node *a, const struct bpf_rb_node *b)
|
|
{
|
|
struct node_data *node_a;
|
|
struct node_data *node_b;
|
|
|
|
node_a = container_of(a, struct node_data, node);
|
|
node_b = container_of(b, struct node_data, node);
|
|
bpf_rbtree_add(&groot, &node_a->node, less);
|
|
|
|
return node_a->key < node_b->key;
|
|
}
|
|
|
|
static bool less__bad_fn_call_remove(struct bpf_rb_node *a, const struct bpf_rb_node *b)
|
|
{
|
|
struct node_data *node_a;
|
|
struct node_data *node_b;
|
|
|
|
node_a = container_of(a, struct node_data, node);
|
|
node_b = container_of(b, struct node_data, node);
|
|
bpf_rbtree_remove(&groot, &node_a->node);
|
|
|
|
return node_a->key < node_b->key;
|
|
}
|
|
|
|
static bool less__bad_fn_call_first_unlock_after(struct bpf_rb_node *a, const struct bpf_rb_node *b)
|
|
{
|
|
struct node_data *node_a;
|
|
struct node_data *node_b;
|
|
|
|
node_a = container_of(a, struct node_data, node);
|
|
node_b = container_of(b, struct node_data, node);
|
|
bpf_rbtree_first(&groot);
|
|
bpf_spin_unlock(&glock);
|
|
|
|
return node_a->key < node_b->key;
|
|
}
|
|
|
|
static __always_inline
|
|
long add_with_cb(bool (cb)(struct bpf_rb_node *a, const struct bpf_rb_node *b))
|
|
{
|
|
struct node_data *n;
|
|
|
|
n = bpf_obj_new(typeof(*n));
|
|
if (!n)
|
|
return 1;
|
|
|
|
bpf_spin_lock(&glock);
|
|
bpf_rbtree_add(&groot, &n->node, cb);
|
|
bpf_spin_unlock(&glock);
|
|
return 0;
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("arg#1 expected pointer to allocated object")
|
|
long rbtree_api_add_bad_cb_bad_fn_call_add(void *ctx)
|
|
{
|
|
return add_with_cb(less__bad_fn_call_add);
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("rbtree_remove not allowed in rbtree cb")
|
|
long rbtree_api_add_bad_cb_bad_fn_call_remove(void *ctx)
|
|
{
|
|
return add_with_cb(less__bad_fn_call_remove);
|
|
}
|
|
|
|
SEC("?tc")
|
|
__failure __msg("can't spin_{lock,unlock} in rbtree cb")
|
|
long rbtree_api_add_bad_cb_bad_fn_call_first_unlock_after(void *ctx)
|
|
{
|
|
return add_with_cb(less__bad_fn_call_first_unlock_after);
|
|
}
|
|
|
|
char _license[] SEC("license") = "GPL";
|