Registrating struct_ops types from modules

static const struct bpf_struct_ops *

kernel/bpf/bpf_struct_ops.c +1 -1

-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
  {
  	unsigned int i;

Quoted 5 lines · On 10/17/23 9:22 AM, [email protected] wrote:

  static const struct bpf_struct_ops *

kernel/bpf/bpf_struct_ops.c +1 -1

-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(struct btf *btf, u32 value_id)

  {
      unsigned int i;

[ ... ]

kernel/bpf/bpf_struct_ops.c static struct bpf_map · line 676

Quoted 7 lines

kernel/bpf/bpf_struct_ops.c +1 -1

@@ -671,7 +672,7 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_map *map;
       int ret;
-    st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
+    st_ops = 
 bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);

This patch does not compile because of the argument ordering.

kernel/bpf/btf.c bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log, · line 8672

Quoted 10 lines · On 10/17/23 9:22 AM, [email protected] wrote:

+const struct bpf_struct_ops **btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)

kernel/bpf/btf.c +8 -0

+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)
+        return NULL;
+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops **)btf->struct_ops_tab->ops;

Is it possible that the module is already gone here? If that is the case, the st_ops pointer probably cannot be used?

From: Kui-Feng Lee <[email protected]>

To ensure that a module remains accessible whenever a struct_ops object of a struct_ops type provided by the module is still in use.

Signed-off-by Kui-Feng Lee <[email protected]>

---
  include/linux/bpf.h         |  1 +
  kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
  2 files changed, 19 insertions(+), 3 deletions(-)

include/linux/bpf.h +1 -0

index e6a648af2daa..1e1647c8b0ce 100644
@@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
  	int (*update)(void *kdata, void *old_kdata);
  	int (*validate)(void *kdata);
  	struct btf *btf;
+	struct module *owner;
  	const struct btf_type *type;
  	const struct btf_type *value_type;
  	const char *name;

kernel/bpf/bpf_struct_ops.c +1 -0

index 7758f66ad734..b561245fe235 100644
@@ -112,6 +112,7 @@ static const struct btf_type *module_type;
 
  static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
  				    struct btf *btf,
+				    struct module *owner,
  				    struct bpf_verifier_log *log)
  {
  	const struct btf_member *member;

kernel/bpf/bpf_struct_ops.c +2 -1

  }
 
  void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 
  	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
  		st_ops = bpf_struct_ops[i];
-		bpf_struct_ops_init_one(st_ops, btf, log);
+		bpf_struct_ops_init_one(st_ops, btf, NULL, log);
  	}
  }
 

@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
  		bpf_jit_uncharge_modmem(PAGE_SIZE);
  	}
  	bpf_map_area_free(st_map->uvalue);
+	module_put(st_map->st_ops->owner);
  	bpf_map_area_free(st_map);
  }
 

kernel/bpf/bpf_struct_ops.c +8 -2

+		return ERR_PTR(-EINVAL);
+
  	vt = st_ops->value_type;
-	if (attr->value_size != vt->size)
+	if (attr->value_size != vt->size) {
+		module_put(st_ops->owner);
  		return ERR_PTR(-EINVAL);
+	}
 
  	t = st_ops->type;
 

@@ -689,8 +702,10 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  		(vt->size - sizeof(struct bpf_struct_ops_value));
 
  	st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
-	if (!st_map)
+	if (!st_map) {
+		module_put(st_ops->owner);
  		return ERR_PTR(-ENOMEM);
+	}
 
  	st_map->st_ops = st_ops;
  	map = &st_map->map;

Quoted 34 lines · On 10/17/23 9:23 AM, [email protected] wrote:

From: Kui-Feng Lee <[email protected]>

To ensure that a module remains accessible whenever a struct_ops object of a struct_ops type provided by the module is still in use.

Signed-off-by Kui-Feng Lee <[email protected]>

---
  include/linux/bpf.h         |  1 + kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---

  2 files changed, 19 insertions(+), 3 deletions(-)

include/linux/bpf.h +1 -0

index e6a648af2daa..1e1647c8b0ce 100644
@@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
       int (*update)(void *kdata, void *old_kdata);
       int (*validate)(void *kdata);
       struct btf *btf;
+    struct module *owner;
       const struct btf_type *type;
       const struct btf_type *value_type;
       const char *name;

kernel/bpf/bpf_struct_ops.c +1 -0

index 7758f66ad734..b561245fe235 100644
@@ -112,6 +112,7 @@ static const struct btf_type *module_type;
   static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
                       struct btf *btf,
+                    struct module *owner,
                       struct bpf_verifier_log *log)
   {
       const struct btf_member *member;

kernel/bpf/bpf_struct_ops.c static void bpf_struct_ops_init_one(struct · line 191

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct 
 bpf_struct_ops *st_ops,
                   st_ops->name);
           } else {
               st_ops->btf = btf;
+            st_ops->owner = owner;

I suspect it will turn out to be just "st_ops->owner = st_ops->owner;" in a latter patch. st_ops->owner should have already been initialized (with THIS_MODULE?).

kernel/bpf/bpf_struct_ops.c static void bpf_struct_ops_init_one(struct · line 199

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +0 -0

               st_ops->type_id = type_id;
               st_ops->type = t;

              st_ops->value_id = value_id;

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct 
 bpf_struct_ops *st_ops,
                                   value_id);
           }
       }
+

nit. extra newline.

Quoted 22 lines

kernel/bpf/bpf_struct_ops.c +2 -1

   }
   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
 bpf_verifier_log *log)
       for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
           st_ops = bpf_struct_ops[i];
-        bpf_struct_ops_init_one(st_ops, btf, log);
+        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
       }
   }

@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
 bpf_map *map)
           bpf_jit_uncharge_modmem(PAGE_SIZE);
       }
       bpf_map_area_free(st_map->uvalue);
+    module_put(st_map->st_ops->owner);
       bpf_map_area_free(st_map);
   }

@@ -676,9 +680,18 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       if (!st_ops)

kernel/bpf/bpf_struct_ops.c static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) · line 687

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +5 -0

           return ERR_PTR(-ENOTSUPP);
+    /* If st_ops->owner is NULL, it means the struct_ops is
+     * statically defined in the kernel.  We don't need to
+     * take a refcount on it.
+     */
+    if (st_ops->owner && !btf_try_get_module(st_ops->btf))

This just came to my mind. Is the module refcnt needed during map alloc/free or it could be done during the reg/unreg instead?

Quoted 22 lines

kernel/bpf/bpf_struct_ops.c +8 -2

+        return ERR_PTR(-EINVAL);
+
       vt = st_ops->value_type;
-    if (attr->value_size != vt->size)
+    if (attr->value_size != vt->size) {
+        module_put(st_ops->owner);
           return ERR_PTR(-EINVAL);
+    }
       t = st_ops->type;

@@ -689,8 +702,10 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
           (vt->size - sizeof(struct bpf_struct_ops_value));
       st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
-    if (!st_map)
+    if (!st_map) {
+        module_put(st_ops->owner);
           return ERR_PTR(-ENOMEM);
+    }
       st_map->st_ops = st_ops;
       map = &st_map->map;

Quoted 24 lines · Martin KaFai Lau

Quoted 21 lines · On 10/17/23 9:23 AM, [email protected] wrote:

  }
  void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

kernel/bpf/bpf_struct_ops.c +2 -1

@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
 bpf_verifier_log *log)
       for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
           st_ops = bpf_struct_ops[i];
-        bpf_struct_ops_init_one(st_ops, btf, log);
+        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
       }
   }

@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
 bpf_map *map)
           bpf_jit_uncharge_modmem(PAGE_SIZE);
       }
       bpf_map_area_free(st_map->uvalue);
+    module_put(st_map->st_ops->owner);
       bpf_map_area_free(st_map);
   }

@@ -676,9 +680,18 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       if (!st_ops)

Quoted 8 lines

kernel/bpf/bpf_struct_ops.c static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) · line 687

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +5 -0

           return ERR_PTR(-ENOTSUPP);
+    /* If st_ops->owner is NULL, it means the struct_ops is
+     * statically defined in the kernel.  We don't need to
+     * take a refcount on it.
+     */
+    if (st_ops->owner && !btf_try_get_module(st_ops->btf))

This just came to my mind. Is the module refcnt needed during map alloc/free or it could be done during the reg/unreg instead?

Sure, I can move it to reg/unreg.

Quoted 24 lines · Kui-Feng Lee

Quoted 21 lines · Martin KaFai Lau

Quoted 18 lines · On 10/17/23 9:23 AM, [email protected] wrote:

  }
  void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

kernel/bpf/bpf_struct_ops.c +2 -1

@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
 bpf_verifier_log *log)
       for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
           st_ops = bpf_struct_ops[i];
-        bpf_struct_ops_init_one(st_ops, btf, log);
+        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
       }
   }

@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
           bpf_jit_uncharge_modmem(PAGE_SIZE);
       }
       bpf_map_area_free(st_map->uvalue);
+    module_put(st_map->st_ops->owner);
       bpf_map_area_free(st_map);
   }

@@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union 

Quoted 8 lines

Quoted 8 lines

kernel/bpf/bpf_struct_ops.c static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) · line 687

Quoted 8 lines

kernel/bpf/bpf_struct_ops.c +5 -0

 bpf_attr *attr)
       if (!st_ops)
           return ERR_PTR(-ENOTSUPP);
+    /* If st_ops->owner is NULL, it means the struct_ops is
+     * statically defined in the kernel.  We don't need to
+     * take a refcount on it.
+     */
+    if (st_ops->owner && !btf_try_get_module(st_ops->btf))

Quoted 4 lines

Quoted 2 lines

This just came to my mind. Is the module refcnt needed during map alloc/free or it could be done during the reg/unreg instead?

Quoted 1 line

Sure, I can move it to reg/unreg.

Just found that we relies type information in st_ops to update element and clean up maps. We can not move get/put modules to reg/unreg except keeping a redundant copy in st_map or somewhere. It make the code much more complicated by
introducing get/put module here and there.

I prefer to keep as it is now. WDYT?

Quoted 30 lines · Kui-Feng Lee

Quoted 27 lines · Kui-Feng Lee

Quoted 24 lines · Martin KaFai Lau

Quoted 21 lines · On 10/17/23 9:23 AM, [email protected] wrote:

  }
  void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

kernel/bpf/bpf_struct_ops.c +2 -1

@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, 
 struct bpf_verifier_log *log)
       for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
           st_ops = bpf_struct_ops[i];
-        bpf_struct_ops_init_one(st_ops, btf, log);
+        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
       }
   }

@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
 bpf_map *map)
           bpf_jit_uncharge_modmem(PAGE_SIZE);
       }
       bpf_map_area_free(st_map->uvalue);
+    module_put(st_map->st_ops->owner);
       bpf_map_area_free(st_map);
   }

@@ -676,9 +680,18 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       if (!st_ops)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) · line 687

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +5 -0

           return ERR_PTR(-ENOTSUPP);
+    /* If st_ops->owner is NULL, it means the struct_ops is
+     * statically defined in the kernel.  We don't need to
+     * take a refcount on it.
+     */
+    if (st_ops->owner && !btf_try_get_module(st_ops->btf))

While replying and looking at it again, I don't think the btf_try_get_module(st_ops->btf) is safe. The module's owned st_ops itself could have been gone with the module. The same goes with the "st_ops->owner" test, so btf_is_module(btf) should be used instead.

I am risking to act like a broken clock to repeat this question, does it really need to store btf back into the st_ops which may accidentally get into the above btf_try_get_module(st_ops->btf) usage?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

This just came to my mind. Is the module refcnt needed during map alloc/free or it could be done during the reg/unreg instead?

Quoted 8 lines

Quoted 1 line

Sure, I can move it to reg/unreg.

Just found that we relies type information in st_ops to update element and clean up maps. We can not move get/put modules to reg/unreg except keeping a redundant copy in st_map or somewhere. It make the code much more complicated by
introducing get/put module here and there.

I prefer to keep as it is now. WDYT?

Yeah, sure. I was asking after seeing a longer wait time for the module to go away in patch 11 selftest and requires an explicit waiting for the tasks_trace period. Releasing the module refcnt earlier will help.

Regardless of the module refcnt hold/free location, I think storing the type* and value* in the module's owned st_ops does not look correct now. It was fine and convenient to piggy back them into bpf_struct_ops when everything was built-in the kernel and no lifetime concern. It makes sense now to separate them out from the module's owned st_ops. Something like:

struct btf_struct_ops_desc {
    struct bpf_struct_ops *ops;
        const struct btf_type *type;
        const struct btf_type *value_type;
        u32 type_id;
        u32 value_id;
};

struct btf_struct_ops_tab {
        u32 cnt;
    u32 capacity;
    struct btf_struct_ops_desc *st_ops_desc[];
};

wdyt?

From: Kui-Feng Lee <[email protected]>

Replace the static list of struct_ops types with pre-btf struct_ops_tab to enable dynamic registration.

Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function instead of being listed in bpf_struct_ops_types.h.

Cc [email protected]

Signed-off-by Kui-Feng Lee <[email protected]>

---
  include/linux/bpf.h               |   2 +
  include/linux/btf.h               |  29 +++++++
  kernel/bpf/bpf_struct_ops.c       | 124 +++++++++++++++---------------
  kernel/bpf/bpf_struct_ops_types.h |  12 ---
  kernel/bpf/btf.c                  |   2 +-
  net/bpf/bpf_dummy_struct_ops.c    |  14 +++-
  net/ipv4/bpf_tcp_ca.c             |  16 +++-
  7 files changed, 119 insertions(+), 80 deletions(-)

delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

include/linux/bpf.h +2 -0

index 1e1647c8b0ce..b0f33147aa93 100644
@@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
  	return prog->aux->func_idx != 0;
  }
 
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
+
  #endif /* _LINUX_BPF_H */

include/linux/btf.h +5 -0

index aa2ba77648be..fdc83aa10462 100644
@@ -12,6 +12,8 @@
  #include <uapi/linux/bpf.h>
 
  #define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);	\
+		((void)(struct bpf_struct_ops_##type *)0); }
  #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
 
  /* These need to be macros, as the expressions are used in assembler input */

@@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
  bool btf_is_kernel(const struct btf *btf);
  bool btf_is_module(const struct btf *btf);
  struct module *btf_try_get_module(const struct btf *btf);
+struct btf *btf_get_module_btf(const struct module *module);
  u32 btf_nr_types(const struct btf *btf);
  bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
  			   const struct btf_member *m,

@@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops *st_ops);
  const struct bpf_struct_ops **
  btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
 
+enum bpf_struct_ops_state {
+	BPF_STRUCT_OPS_STATE_INIT,

include/linux/btf.h +1 -0

+
  #endif

kernel/bpf/bpf_struct_ops.c +0 -42

index 60445ff32275..175068b083cb 100644
@@ -13,19 +13,6 @@
  #include <linux/btf_ids.h>
  #include <linux/rcupdate_wait.h>
 
-enum bpf_struct_ops_state {
-	BPF_STRUCT_OPS_STATE_INIT,
-	BPF_STRUCT_OPS_STATE_INUSE,
-	BPF_STRUCT_OPS_STATE_TOBEFREE,
-	BPF_STRUCT_OPS_STATE_READY,
-};
-
-struct bpf_struct_ops_common_value {
-	refcount_t refcnt;
-	enum bpf_struct_ops_state state;
-};
-#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
-
  struct bpf_struct_ops_value {
  	BPF_STRUCT_OPS_COMMON_VALUE;
  	char data[] ____cacheline_aligned_in_smp;

@@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
  #define VALUE_PREFIX "bpf_struct_ops_"
  #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
 
-/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
- * the map's value exposed to the userspace and its btf-type-id is
- * stored at the map->btf_vmlinux_value_type_id.
- *
- */
-#define BPF_STRUCT_OPS_TYPE(_name)				\
-extern struct bpf_struct_ops bpf_##_name;			\
-								\
-struct bpf_struct_ops_##_name {						\
-	BPF_STRUCT_OPS_COMMON_VALUE;				\
-	struct _name data ____cacheline_aligned_in_smp;		\
-};
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-
-enum {
-#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-	__NR_BPF_STRUCT_OPS_TYPE,
-};
-
-static struct bpf_struct_ops * const bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name)				\
-	[BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
  const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
  };
 

kernel/bpf/bpf_struct_ops.c +16 -0

+{
+	struct bpf_verifier_log *log;
+	int err;
+
+	if (st_ops == NULL)
+		return -EINVAL;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+	if (!log) {
+		err = -ENOMEM;
+		goto errout;
+	}
+
+	log->level = BPF_LOG_KERNEL;
+
+	bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);

kernel/bpf/bpf_struct_ops.c +5 -0

+{
+	struct btf *btf;
+	int err;
+
+	btf = btf_get_module_btf(st_ops->owner);

kernel/bpf/bpf_struct_ops.c +0 -12

  {
-	struct bpf_struct_ops *st_ops;
  	s32 module_id, common_value_id;
-	u32 i;
-
-	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
 
  	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
  	if (module_id < 0) {

@@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
  		return;
  	}
  	common_value_type = btf_type_by_id(btf, common_value_id);
-
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		st_ops = bpf_struct_ops[i];
-		bpf_struct_ops_init_one(st_ops, btf, NULL, log);
-	}
  }
 
  extern struct btf *btf_vmlinux;

kernel/bpf/bpf_struct_ops.c +6 -1

+			break;
+		}
  	}
 
-	return NULL;
+	return st_ops;
  }
 
  const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id)
  {
+	const struct bpf_struct_ops *st_ops = NULL;
+	const struct bpf_struct_ops **st_ops_list;
  	unsigned int i;
+	u32 cnt;
 

kernel/bpf/bpf_struct_ops.c +3 -1

+			break;
+		}
  	}
 
-	return NULL;
+	return st_ops;
  }
 
  static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,

kernel/bpf/bpf_struct_ops_types.h deleted +0 -2

deleted file mode 100644
index 5678a9ddf817..000000000000
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* internal file - do not include directly */

kernel/bpf/btf.c +1 -1

index be5144dbb53d..990973d6057d 100644
@@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct btf *btf)
  /* Returns struct btf corresponding to the struct module.
   * This function can return NULL or ERR_PTR.
   */
-static struct btf *btf_get_module_btf(const struct module *module)
+struct btf *btf_get_module_btf(const struct module *module)
  {
  #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
  	struct btf_module *btf_mod, *tmp;

net/bpf/bpf_dummy_struct_ops.c +0 -0

index 5918d1b32e19..724bb7224079 100644

Quoted 62 lines · On 10/17/23 9:23 AM, [email protected] wrote:

From: Kui-Feng Lee <[email protected]>

Replace the static list of struct_ops types with pre-btf struct_ops_tab to enable dynamic registration.

Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function instead of being listed in bpf_struct_ops_types.h.

Cc [email protected]

Signed-off-by Kui-Feng Lee <[email protected]>

---
  include/linux/bpf.h               |   2 + include/linux/btf.h               |  29 +++++++ kernel/bpf/bpf_struct_ops.c       | 124 +++++++++++++++--------------- kernel/bpf/bpf_struct_ops_types.h |  12 --- kernel/bpf/btf.c                  |   2 +- net/bpf/bpf_dummy_struct_ops.c    |  14 +++- net/ipv4/bpf_tcp_ca.c             |  16 +++-

  7 files changed, 119 insertions(+), 80 deletions(-)

  delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

include/linux/bpf.h +2 -0

index 1e1647c8b0ce..b0f33147aa93 100644
@@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct 
 bpf_prog *prog)
       return prog->aux->func_idx != 0;
   }
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
+
   #endif /* _LINUX_BPF_H */

include/linux/btf.h +3 -0

index aa2ba77648be..fdc83aa10462 100644
@@ -12,6 +12,8 @@
   #include <uapi/linux/bpf.h>
   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);    \
+        ((void)(struct bpf_struct_ops_##type *)0); }
   #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
   /* These need to be macros, as the expressions are used in assembler input */

@@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
   bool btf_is_kernel(const struct btf *btf);
   bool btf_is_module(const struct btf *btf);
   struct module *btf_try_get_module(const struct btf *btf);
+struct btf *btf_get_module_btf(const struct module *module);
   u32 btf_nr_types(const struct btf *btf);
   bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
                  const struct btf_member *m,

@@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops 
 *st_ops);

  const struct bpf_struct_ops ** btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);

include/linux/btf.h +7 -0

+enum bpf_struct_ops_state {
+    BPF_STRUCT_OPS_STATE_INIT,
+    BPF_STRUCT_OPS_STATE_INUSE,
+    BPF_STRUCT_OPS_STATE_TOBEFREE,
+    BPF_STRUCT_OPS_STATE_READY,
+};
+

Quoted 6 lines

include/linux/btf.h +5 -0

+struct bpf_struct_ops_common_value {
+    refcount_t refcnt;
+    enum bpf_struct_ops_state state;
+};
+#define BPF_STRUCT_OPS_COMMON_VALUE struct 

bpf_struct_ops_common_value common

Since there is 'struct bpf_struct_ops_common_value' now, the BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets remove it.

Quoted 4 lines

include/linux/btf.h +3 -0

+
+/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
+ * the map's value exposed to the userspace and its btf-type-id is

include/linux/btf.h int btf_add_struct_ops(struct bpf_struct_ops *st_ops); · line 607

Quoted 10 lines

include/linux/btf.h +10 -0

+ * stored at the map->btf_vmlinux_value_type_id.
+ *
+ */
+#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name)            \
+extern struct bpf_struct_ops bpf_##_name;            \
+                                \
+struct bpf_struct_ops_##_name {                    \
+    BPF_STRUCT_OPS_COMMON_VALUE;                \
+    struct _name data ____cacheline_aligned_in_smp;        \
+}

I think the bpp_struct_ops_* should not be in btf.h. Probably move them to bpf.h instead. or there is some other considerations I am missing?

Quoted 60 lines

+
  #endif

kernel/bpf/bpf_struct_ops.c +0 -12

index 60445ff32275..175068b083cb 100644
@@ -13,19 +13,6 @@
   #include <linux/btf_ids.h>
   #include <linux/rcupdate_wait.h>
-enum bpf_struct_ops_state {
-    BPF_STRUCT_OPS_STATE_INIT,
-    BPF_STRUCT_OPS_STATE_INUSE,
-    BPF_STRUCT_OPS_STATE_TOBEFREE,
-    BPF_STRUCT_OPS_STATE_READY,
-};
-
-struct bpf_struct_ops_common_value {
-    refcount_t refcnt;
-    enum bpf_struct_ops_state state;
-};
-#define BPF_STRUCT_OPS_COMMON_VALUE struct 

bpf_struct_ops_common_value common
-
  struct bpf_struct_ops_value {
      BPF_STRUCT_OPS_COMMON_VALUE;
      char data[] ____cacheline_aligned_in_smp;

kernel/bpf/bpf_struct_ops.c +0 -29

@@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
   #define VALUE_PREFIX "bpf_struct_ops_"
   #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
-/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
- * the map's value exposed to the userspace and its btf-type-id is
- * stored at the map->btf_vmlinux_value_type_id.
- *
- */
-#define BPF_STRUCT_OPS_TYPE(_name)                \
-extern struct bpf_struct_ops bpf_##_name;            \
-                                \
-struct bpf_struct_ops_##_name {                        \
-    BPF_STRUCT_OPS_COMMON_VALUE;                \
-    struct _name data ____cacheline_aligned_in_smp;        \
-};
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-
-enum {
-#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-    __NR_BPF_STRUCT_OPS_TYPE,
-};
-
-static struct bpf_struct_ops * const bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name)                \
-    [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
   const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {

kernel/bpf/bpf_struct_ops.c static void bpf_struct_ops_init_one(struct · line 195

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +2 -0

   };

@@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct 
 bpf_struct_ops *st_ops,
   }
+static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
+                       struct btf *btf)

Please combine this function into register_bpf_struct_ops(). They are both very short.

Quoted 20 lines

kernel/bpf/bpf_struct_ops.c +20 -0

+{
+    struct bpf_verifier_log *log;
+    int err;
+
+    if (st_ops == NULL)
+        return -EINVAL;
+
+    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+    if (!log) {
+        err = -ENOMEM;
+        goto errout;
+    }
+
+    log->level = BPF_LOG_KERNEL;
+
+    bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
+
+    err = btf_add_struct_ops(st_ops);
+
+errout:

kernel/bpf/bpf_struct_ops.c static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops, · line 222

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +6 -0

+    kfree(log);
+
+    return err;
+}
+
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)

Similar to the register kfunc counterpart, can this be moved to btf.c instead by extern-ing bpf_struct_ops_init_one()? or there are some other structs/functions need to extern?

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +9 -0

+{
+    struct btf *btf;
+    int err;
+
+    btf = btf_get_module_btf(st_ops->owner);
+    if (!btf)
+        return -EINVAL;
+    err = register_bpf_struct_ops_btf(st_ops, btf);
+    btf_put(btf);

kernel/bpf/bpf_struct_ops.c static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops, · line 237

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +5 -0

+
+    return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
+
   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

The bpf_struct_ops_init() is pretty much only finding the btf "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it instead. Then the newly added bpf_struct_ops_init_one() could use a proper name bpf_struct_ops_init() instead of having the special "_one" suffix.

Quoted 28 lines

kernel/bpf/bpf_struct_ops.c +0 -12

   {
-    struct bpf_struct_ops *st_ops;
       s32 module_id, common_value_id;
-    u32 i;
-
-    /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct 
 bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
       module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
       if (module_id < 0) {

@@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct 
 bpf_verifier_log *log)
           return;
       }
       common_value_type = btf_type_by_id(btf, common_value_id);
-
-    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-        st_ops = bpf_struct_ops[i];
-        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
-    }
   }
   extern struct btf *btf_vmlinux;

@@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;

  static const struct bpf_struct_ops * bpf_struct_ops_find_value(struct btf *btf, u32 value_id)

Quoted 6 lines

  {
+    const struct bpf_struct_ops *st_ops = NULL;
+    const struct bpf_struct_ops **st_ops_list;
      unsigned int i;
+    u32 cnt = 0;
      if (!value_id || !btf_vmlinux)

The "!btf_vmlinux" should have been changed to "!btf" in the earlier patch (patch 2?),

and is this null check still needed now?

Quoted 8 lines

          return NULL;
-    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { -        if (bpf_struct_ops[i]->value_id == value_id) -            return bpf_struct_ops[i];
+    st_ops_list = btf_get_struct_ops(btf, &cnt); +    for (i = 0; i < cnt; i++) {
+        if (st_ops_list[i]->value_id == value_id) { +            st_ops = st_ops_list[i];

nit. Like the change in the earlier patch that is being replaced here, directly "return st_ops_list[i];".

Quoted 14 lines

+            break;
+        }
      }
-    return NULL;
+    return st_ops;
  }
  const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id) {
+    const struct bpf_struct_ops *st_ops = NULL;
+    const struct bpf_struct_ops **st_ops_list;
      unsigned int i;
+    u32 cnt;
      if (!type_id || !btf_vmlinux)

Quoted 8 lines

          return NULL;
-    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { -        if (bpf_struct_ops[i]->type_id == type_id) -            return bpf_struct_ops[i];
+    st_ops_list = btf_get_struct_ops(btf, &cnt); +    for (i = 0; i < cnt; i++) {
+        if (st_ops_list[i]->type_id == type_id) { +            st_ops = st_ops_list[i];

Same.

Quoted 16 lines

+            break;
+        }
      }
-    return NULL;
+    return st_ops;
  }
  static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,

kernel/bpf/bpf_struct_ops_types.h deleted +0 -2

deleted file mode 100644
index 5678a9ddf817..000000000000
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* internal file - do not include directly */

kernel/bpf/bpf_struct_ops_types.h line 12

Quoted 10 lines

kernel/bpf/bpf_struct_ops_types.h deleted +0 -10

-
-#ifdef CONFIG_BPF_JIT
-#ifdef CONFIG_NET
-BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
-#endif
-#ifdef CONFIG_INET
-#include <net/tcp.h>
-BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
-#endif
-#endif

Seeing this gone is satisfying

Quoted 17 lines

kernel/bpf/btf.c +1 -1

index be5144dbb53d..990973d6057d 100644
@@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct 
 btf *btf)
   /* Returns struct btf corresponding to the struct module.
    * This function can return NULL or ERR_PTR.
    */
-static struct btf *btf_get_module_btf(const struct module *module)
+struct btf *btf_get_module_btf(const struct module *module)
   {
   #ifdef CONFIG_DEBUG_INFO_BTF_MODULES

      struct btf_module *btf_mod, *tmp;

net/bpf/bpf_dummy_struct_ops.c +0 -1

index 5918d1b32e19..724bb7224079 100644
--- a/net/bpf/bpf_dummy_struct_ops.c

net/bpf/bpf_dummy_struct_ops.c line 9

Quoted 6 lines

+++ b/net/bpf/bpf_dummy_struct_ops.c

net/bpf/bpf_dummy_struct_ops.c +1 -1

@@ -7,7 +7,7 @@
   #include <linux/bpf.h>
   #include <linux/btf.h>
-extern struct bpf_struct_ops bpf_bpf_dummy_ops;
+static struct bpf_struct_ops bpf_bpf_dummy_ops;

Is it still needed ?

Quoted 11 lines · Martin KaFai Lau

Quoted 9 lines · On 10/17/23 9:23 AM, [email protected] wrote:

From: Kui-Feng Lee <[email protected]>
  static const struct bpf_struct_ops * bpf_struct_ops_find_value(struct btf *btf, u32 value_id) {
+    const struct bpf_struct_ops *st_ops = NULL; +    const struct bpf_struct_ops **st_ops_list; unsigned int i;
+    u32 cnt = 0;
      if (!value_id || !btf_vmlinux)

Quoted 1 line

The "!btf_vmlinux" should have been changed to "!btf" in the earlier patch (patch 2?),

This is not btf. It mean to check if btf_vmlinux is initialized. It is not necessary anymore.
For checking btf, the following btf_get_struct_ops() will keep cnt zero if btf is NULL, so it is unnecessary as well.

From: Kui-Feng Lee <[email protected]>

Locate the module BTFs for struct_ops maps and progs and pass them to the kernel. This ensures that the kernel correctly resolves type IDs from the appropriate module BTFs.

For the map of a struct_ops object, mod_btf is added to bpf_map to keep a reference to the module BTF. The FD of the module BTF is passed to the kernel as mod_btf_fd when the struct_ops object is loaded.

For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a module BTF in the kernel.

Signed-off-by Kui-Feng Lee <[email protected]>

---
 tools/lib/bpf/bpf.c    |  4 ++-
 tools/lib/bpf/bpf.h    |  5 +++-
 tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
 3 files changed, 51 insertions(+), 26 deletions(-)

tools/lib/bpf/bpf.c +3 -1

index b0f1913763a3..af46488e4ea9 100644
@@ -169,7 +169,8 @@ int bpf_map_create(enum bpf_map_type map_type,
                   __u32 max_entries,
                   const struct bpf_map_create_opts *opts)
 {
-       const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
+       const size_t attr_sz = offsetofend(union bpf_attr,
+                                          value_type_btf_obj_fd);
        union bpf_attr attr;
        int fd;
 

@@ -191,6 +192,7 @@ int bpf_map_create(enum bpf_map_type map_type,
        attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
        attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
        attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
+       attr.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_fd, 0);
 
        attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
        attr.map_flags = OPTS_GET(opts, map_flags, 0);

tools/lib/bpf/bpf.h +4 -1

index 74c2887cfd24..1733cdc21241 100644
@@ -51,8 +51,11 @@ struct bpf_map_create_opts {
 
        __u32 numa_node;
        __u32 map_ifindex;
+
+       __u32 value_type_btf_obj_fd;
+       size_t:0;
 };
-#define bpf_map_create_opts__last_field map_ifindex
+#define bpf_map_create_opts__last_field value_type_btf_obj_fd
 
 LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
                              const char *map_name,

tools/lib/bpf/libbpf.c +1 -0

index 3a6108e3238b..d8a60fb52f5c 100644
@@ -519,6 +519,7 @@ struct bpf_map {
        struct bpf_map_def def;
        __u32 numa_node;
        __u32 btf_var_idx;
+       int mod_btf_fd;
        __u32 btf_key_type_id;
        __u32 btf_value_type_id;
        __u32 btf_vmlinux_value_type_id;

tools/lib/bpf/libbpf.c +0 -0

 static const struct btf_member *
 find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
 {

@@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
        return NULL;
 }
 

tools/lib/bpf/libbpf.c +0 -0

 {
        struct module_btf *mod_btf;
        struct btf *btf;

Quoted 14 lines · On Tue, Oct 17, 2023 at 9:23 AM <[email protected]> wrote:

From: Kui-Feng Lee <[email protected]>

Locate the module BTFs for struct_ops maps and progs and pass them to the kernel. This ensures that the kernel correctly resolves type IDs from the appropriate module BTFs.

For the map of a struct_ops object, mod_btf is added to bpf_map to keep a reference to the module BTF. The FD of the module BTF is passed to the kernel as mod_btf_fd when the struct_ops object is loaded.

For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a module BTF in the kernel.

Quoted 7 lines

Signed-off-by Kui-Feng Lee <[email protected]>

---
  tools/lib/bpf/bpf.c    |  4 ++-
  tools/lib/bpf/bpf.h    |  5 +++-
  tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
  3 files changed, 51 insertions(+), 26 deletions(-)

I have a few nits, please accommodate them, and with that please add my ack on libbpf side of things

Acked-by Andrii Nakryiko <[email protected]>

Quoted 56 lines

tools/lib/bpf/bpf.c +3 -1

index b0f1913763a3..af46488e4ea9 100644
@@ -169,7 +169,8 @@ int bpf_map_create(enum bpf_map_type map_type,
                    __u32 max_entries,
                    const struct bpf_map_create_opts *opts)
  {
-       const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
+       const size_t attr_sz = offsetofend(union bpf_attr,
+                                          value_type_btf_obj_fd);
         union bpf_attr attr;
         int fd;
 

@@ -191,6 +192,7 @@ int bpf_map_create(enum bpf_map_type map_type,
         attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
         attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
         attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
+       attr.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_fd, 0);
 
         attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
         attr.map_flags = OPTS_GET(opts, map_flags, 0);

tools/lib/bpf/bpf.h +4 -1

index 74c2887cfd24..1733cdc21241 100644
@@ -51,8 +51,11 @@ struct bpf_map_create_opts {
 
         __u32 numa_node;
         __u32 map_ifindex;
+
+       __u32 value_type_btf_obj_fd;
+       size_t:0;
  };
-#define bpf_map_create_opts__last_field map_ifindex
+#define bpf_map_create_opts__last_field value_type_btf_obj_fd
 
  LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
                               const char *map_name,

tools/lib/bpf/libbpf.c +1 -0

index 3a6108e3238b..d8a60fb52f5c 100644
@@ -519,6 +519,7 @@ struct bpf_map {
         struct bpf_map_def def;
         __u32 numa_node;
         __u32 btf_var_idx;
+       int mod_btf_fd;
         __u32 btf_key_type_id;
         __u32 btf_value_type_id;
         __u32 btf_vmlinux_value_type_id;

@@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
         return 0;
  }
 

tools/lib/bpf/libbpf.c bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, · line 898

Quoted 2 lines

tools/lib/bpf/libbpf.c +2 -0

+static int load_module_btfs(struct bpf_object *obj);
+

you don't need this forward declaration, do you?

Quoted 8 lines

tools/lib/bpf/libbpf.c +0 -0

  static const struct btf_member *
  find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
  {

@@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
         return NULL;
  }
 

[...]

Quoted 6 lines

         if (obj->btf && btf__fd(obj->btf) >= 0) {
                 create_attr.btf_fd = btf__fd(obj->btf);

tools/lib/bpf/libbpf.c +0 -0

@@ -7700,9 +7718,9 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
         return libbpf_kallsyms_parse(kallsyms_cb, obj);
  }
 

tools/lib/bpf/libbpf.c static int bpf_object__read_kallsyms_file(struct bpf_object *obj) · line 7723

Quoted 6 lines

tools/lib/bpf/libbpf.c +3 -3

-static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
-                           __u16 kind, struct btf **res_btf,
-                           struct module_btf **res_mod_btf)
+static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
+                             __u16 kind, struct btf **res_btf,
+                             struct module_btf **res_mod_btf)

I actually find "find_module" terminology confusing, because it might not be in the module after all, right? I think "find_ksym_btf_id" is a totally appropriate name, and it's just that in some cases that kernel symbol (ksym) will be found in the kernel module instead of in vmlinux image itself. Still, it's a kernel. Let's keep the name?

Quoted 4 lines

tools/lib/bpf/libbpf.c +0 -0

  {
         struct module_btf *mod_btf;
         struct btf *btf;

[...]

tools/lib/bpf/libbpf.c +14 -12

-static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
-			    __u16 kind, struct btf **res_btf,
-			    struct module_btf **res_mod_btf)
+static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
+			      __u16 kind, struct btf **res_btf,
+			      struct module_btf **res_mod_btf)
  {
  	struct module_btf *mod_btf;
  	struct btf *btf;

@@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
 
  	btf = obj->btf_vmlinux;
  	mod_btf = NULL;
-	id = btf__find_by_name_kind(btf, ksym_name, kind);
+	id = btf__find_by_name_kind(btf, kern_name, kind);
 
  	if (id == -ENOENT) {
  		err = load_module_btfs(obj);

@@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
  			/* we assume module_btf's BTF FD is always >0 */
  			mod_btf = &obj->btf_modules[i];
  			btf = mod_btf->btf;
-			id = btf__find_by_name_kind_own(btf, ksym_name, kind);
+			id = btf__find_by_name_kind_own(btf, kern_name, kind);
  			if (id != -ENOENT)
  				break;
  		}

@@ -7744,7 +7762,7 @@ static int bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
  	struct btf *btf = NULL;
  	int id, err;
 
-	id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
+	id = find_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
  	if (id < 0) {
  		if (id == -ESRCH && ext->is_weak)
  			return 0;

@@ -7798,8 +7816,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
 
  	local_func_proto_id = ext->ksym.type_id;
 
-	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
-				    &mod_btf);
+	kfunc_id = find_module_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
+				      &mod_btf);
  	if (kfunc_id < 0) {
  		if (kfunc_id == -ESRCH && ext->is_weak)
  			return 0;

@@ -9464,9 +9482,9 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
  	return err;
  }
 
-static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
-			      enum bpf_attach_type attach_type,
-			      int *btf_obj_fd, int *btf_type_id)
+static int find_kernel_attach_btf_id(struct bpf_object *obj, const char *attach_name,
+				     enum bpf_attach_type attach_type,
+				     int *btf_obj_fd, int *btf_type_id)
  {
  	int ret, i;
 

@@ -9531,7 +9549,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, const char *attac
  		*btf_obj_fd = 0;
  		*btf_type_id = 1;
  	} else {
-		err = find_kernel_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
+		err = find_kernel_attach_btf_id(prog->obj, attach_name,
+						attach_type, btf_obj_fd,
+						btf_type_id);
  	}
  	if (err) {
  		pr_warn("prog '%s': failed to find kernel BTF type ID of '%s': %d\n",

Quoted 83 lines · On 10/17/23 9:23 AM, [email protected] wrote:

-static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
-                __u16 kind, struct btf **res_btf,
-                struct module_btf **res_mod_btf) +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
+                  __u16 kind, struct btf **res_btf,
+                  struct module_btf **res_mod_btf)
  {
      struct module_btf *mod_btf;
      struct btf *btf;

tools/lib/bpf/libbpf.c +2 -2

@@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object 
 *obj, const char *ksym_name,
       btf = obj->btf_vmlinux;
       mod_btf = NULL;
-    id = btf__find_by_name_kind(btf, ksym_name, kind);
+    id = btf__find_by_name_kind(btf, kern_name, kind);
       if (id == -ENOENT) {
           err = load_module_btfs(obj);

@@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object 
 *obj, const char *ksym_name,
               /* we assume module_btf's BTF FD is always >0 */
               mod_btf = &obj->btf_modules[i];
               btf = mod_btf->btf;
-            id = btf__find_by_name_kind_own(btf, ksym_name, kind);
+            id = btf__find_by_name_kind_own(btf, kern_name, kind);
               if (id != -ENOENT)
                   break;

          }

tools/lib/bpf/libbpf.c +1 -1

@@ -7744,7 +7762,7 @@ static int 
 bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
       struct btf *btf = NULL;
       int id, err;
-    id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
+    id = find_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, 
 &mod_btf);
       if (id < 0) {
           if (id == -ESRCH && ext->is_weak)

              return 0;

tools/lib/bpf/libbpf.c +2 -2

@@ -7798,8 +7816,8 @@ static int 
 bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
       local_func_proto_id = ext->ksym.type_id;
-    kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, 
 BTF_KIND_FUNC, &kern_btf,
-                    &mod_btf);
+    kfunc_id = find_module_btf_id(obj, ext->essent_name ?: ext->name, 
 BTF_KIND_FUNC, &kern_btf,
+                      &mod_btf);
       if (kfunc_id < 0) {
           if (kfunc_id == -ESRCH && ext->is_weak)

              return 0;

tools/lib/bpf/libbpf.c +3 -3

@@ -9464,9 +9482,9 @@ static int libbpf_find_prog_btf_id(const char 
 *name, __u32 attach_prog_fd)
       return err;
   }
-static int find_kernel_btf_id(struct bpf_object *obj, const char 
 *attach_name,
-                  enum bpf_attach_type attach_type,
-                  int *btf_obj_fd, int *btf_type_id)
+static int find_kernel_attach_btf_id(struct bpf_object *obj, const 
 char *attach_name,
+                     enum bpf_attach_type attach_type,
+                     int *btf_obj_fd, int *btf_type_id)
   {

      int ret, i;

(quoted diff) +0 -0

@@ -9531,7 +9549,9 @@ static int libbpf_find_attach_btf_id(struct 

bpf_program *prog, const char *attac
          *btf_obj_fd = 0;
          *btf_type_id = 1;
      } else {
-        err = find_kernel_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
+        err = find_kernel_attach_btf_id(prog->obj, attach_name,
+                        attach_type, btf_obj_fd,
+                        btf_type_id);
      }
      if (err) {
          pr_warn("prog '%s': failed to find kernel BTF type ID of '%s': %d\n",

tools/lib/bpf/libbpf.c +0 -0

@@ -12945,9 +12965,9 @@ int bpf_program__set_attach_target(struct 
 bpf_program *prog,
           err = bpf_object__load_vmlinux_btf(prog->obj, true);
           if (err)
               return libbpf_err(err);

tools/lib/bpf/libbpf.c int bpf_program__set_attach_target(struct bpf_program *prog, · line 12970

Quoted 6 lines

tools/lib/bpf/libbpf.c +3 -3

-        err = find_kernel_btf_id(prog->obj, attach_func_name,
-                     prog->expected_attach_type,
-                     &btf_obj_fd, &btf_id);
+        err = find_kernel_attach_btf_id(prog->obj, attach_func_name,
+                        prog->expected_attach_type,
+                        &btf_obj_fd, &btf_id);

Please avoid mixing this level of name changes with the main changes. It is quite confusing for the reviewer and it is not mentioned in the commit message either.