[bpf-next,2/6] bpf: Introduce internal definitions for UAPI-opaque bpf_{rb,list}_node

Commit Message

Dave Marchevsky July 11, 2023, 5:59 p.m. UTC

Structs bpf_rb_node and bpf_list_node are opaquely defined in
uapi/linux/bpf.h, as BPF program writers are not expected to touch their
fields - nor does the verifier allow them to do so.

Currently these structs are simple wrappers around structs rb_node and
list_head and linked_list / rbtree implementation just casts and passes
to library functions for those data structures. Later patches in this
series, though, will add an "owner" field to bpf_{rb,list}_node, such
that they're not just wrapping an underlying node type. Moreover, the
bpf linked_list and rbtree implementations will deal with these owner
pointers directly in a few different places.

To avoid having to do

  void *owner = (void*)bpf_list_node + sizeof(struct list_head)

with opaque UAPI node types, add bpf_{list,rb}_node_internal struct
definitions to internal headers and modify linked_list and rbtree to use
the internal types where appropriate.

Signed-off-by: Dave Marchevsky <[e-mail]>
---
 include/linux/bpf.h  | 10 ++++++++++
 kernel/bpf/helpers.c | 23 +++++++++++++----------
 2 files changed, 23 insertions(+), 10 deletions(-)

Comments

Alexei Starovoitov July 11, 2023, 8:51 p.m. UTC | #1

On Tue, Jul 11, 2023 at 11:00 AM Dave Marchevsky <[e-mail]> wrote:
>
> Structs bpf_rb_node and bpf_list_node are opaquely defined in
> uapi/linux/bpf.h, as BPF program writers are not expected to touch their
> fields - nor does the verifier allow them to do so.
>
> Currently these structs are simple wrappers around structs rb_node and
> list_head and linked_list / rbtree implementation just casts and passes
> to library functions for those data structures. Later patches in this
> series, though, will add an "owner" field to bpf_{rb,list}_node, such
> that they're not just wrapping an underlying node type. Moreover, the
> bpf linked_list and rbtree implementations will deal with these owner
> pointers directly in a few different places.
>
> To avoid having to do
>
>   void *owner = (void*)bpf_list_node + sizeof(struct list_head)
>
> with opaque UAPI node types, add bpf_{list,rb}_node_internal struct
> definitions to internal headers and modify linked_list and rbtree to use
> the internal types where appropriate.
>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> ---
>  include/linux/bpf.h  | 10 ++++++++++
>  kernel/bpf/helpers.c | 23 +++++++++++++----------
>  2 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 360433f14496..d5841059fd2f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -228,6 +228,16 @@ struct btf_record {
>         struct btf_field fields[];
>  };
>
> +/* Non-opaque version of bpf_rb_node in uapi/linux/bpf.h */
> +struct bpf_rb_node_internal {
> +       struct rb_node rb_node;
> +} __attribute__((aligned(8)));
> +
> +/* Non-opaque version of bpf_list_node in uapi/linux/bpf.h */
> +struct bpf_list_node_internal {
> +       struct list_head list_head;
> +} __attribute__((aligned(8)));

We typically use _kern suffix for data structs that
mirror bpf.h structs.
Let's use it here as well instead of _internal.

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 360433f14496..d5841059fd2f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -228,6 +228,16 @@  struct btf_record {
 	struct btf_field fields[];
 };
 
+/* Non-opaque version of bpf_rb_node in uapi/linux/bpf.h */
+struct bpf_rb_node_internal {
+	struct rb_node rb_node;
+} __attribute__((aligned(8)));
+
+/* Non-opaque version of bpf_list_node in uapi/linux/bpf.h */
+struct bpf_list_node_internal {
+	struct list_head list_head;
+} __attribute__((aligned(8)));
+
 struct bpf_map {
 	/* The first two cachelines with read-mostly members of which some
 	 * are also accessed in fast-path (e.g. ops, max_entries).
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 9e80efa59a5d..f059adefbe82 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1942,10 +1942,11 @@  __bpf_kfunc void *bpf_refcount_acquire_impl(void *p__refcounted_kptr, void *meta
 	return (void *)p__refcounted_kptr;
 }
 
-static int __bpf_list_add(struct bpf_list_node *node, struct bpf_list_head *head,
+static int __bpf_list_add(struct bpf_list_node_internal *node,
+			  struct bpf_list_head *head,
 			  bool tail, struct btf_record *rec, u64 off)
 {
-	struct list_head *n = (void *)node, *h = (void *)head;
+	struct list_head *n = &node->list_head, *h = (void *)head;
 
 	/* If list_head was 0-initialized by map, bpf_obj_init_field wasn't
 	 * called on its fields, so init here
@@ -1967,20 +1968,20 @@  __bpf_kfunc int bpf_list_push_front_impl(struct bpf_list_head *head,
 					 struct bpf_list_node *node,
 					 void *meta__ign, u64 off)
 {
+	struct bpf_list_node_internal *n = (void *)node;
 	struct btf_struct_meta *meta = meta__ign;
 
-	return __bpf_list_add(node, head, false,
-			      meta ? meta->record : NULL, off);
+	return __bpf_list_add(n, head, false, meta ? meta->record : NULL, off);
 }
 
 __bpf_kfunc int bpf_list_push_back_impl(struct bpf_list_head *head,
 					struct bpf_list_node *node,
 					void *meta__ign, u64 off)
 {
+	struct bpf_list_node_internal *n = (void *)node;
 	struct btf_struct_meta *meta = meta__ign;
 
-	return __bpf_list_add(node, head, true,
-			      meta ? meta->record : NULL, off);
+	return __bpf_list_add(n, head, true, meta ? meta->record : NULL, off);
 }
 
 static struct bpf_list_node *__bpf_list_del(struct bpf_list_head *head, bool tail)
@@ -2013,7 +2014,7 @@  __bpf_kfunc struct bpf_rb_node *bpf_rbtree_remove(struct bpf_rb_root *root,
 						  struct bpf_rb_node *node)
 {
 	struct rb_root_cached *r = (struct rb_root_cached *)root;
-	struct rb_node *n = (struct rb_node *)node;
+	struct rb_node *n = &((struct bpf_rb_node_internal *)node)->rb_node;
 
 	if (RB_EMPTY_NODE(n))
 		return NULL;
@@ -2026,11 +2027,12 @@  __bpf_kfunc struct bpf_rb_node *bpf_rbtree_remove(struct bpf_rb_root *root,
 /* Need to copy rbtree_add_cached's logic here because our 'less' is a BPF
  * program
  */
-static int __bpf_rbtree_add(struct bpf_rb_root *root, struct bpf_rb_node *node,
+static int __bpf_rbtree_add(struct bpf_rb_root *root,
+			    struct bpf_rb_node_internal *node,
 			    void *less, struct btf_record *rec, u64 off)
 {
 	struct rb_node **link = &((struct rb_root_cached *)root)->rb_root.rb_node;
-	struct rb_node *parent = NULL, *n = (struct rb_node *)node;
+	struct rb_node *parent = NULL, *n = &node->rb_node;
 	bpf_callback_t cb = (bpf_callback_t)less;
 	bool leftmost = true;
 
@@ -2060,8 +2062,9 @@  __bpf_kfunc int bpf_rbtree_add_impl(struct bpf_rb_root *root, struct bpf_rb_node
 				    void *meta__ign, u64 off)
 {
 	struct btf_struct_meta *meta = meta__ign;
+	struct bpf_rb_node_internal *n = (void *)node;
 
-	return __bpf_rbtree_add(root, node, (void *)less, meta ? meta->record : NULL, off);
+	return __bpf_rbtree_add(root, n, (void *)less, meta ? meta->record : NULL, off);
 }
 
 __bpf_kfunc struct bpf_rb_node *bpf_rbtree_first(struct bpf_rb_root *root)