Registrating struct_ops types from modules

+static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc,
struct btf *btf,
struct bpf_verifier_log *log)

nit. I think this should be renamed to bpf_

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

Maintain a registry of registered struct_ops types in the per-btf (module) struct_ops_tab. This registry allows for easy lookup of struct_ops types that are registered by a specific module.

Every struct_ops type should have an associated module BTF to provide type information since we are going to allow modules to define and register new struct_ops types. Once this change is made, the bpf_struct_ops subsystem knows where to look up type info with just a bpf_struct_ops.

The subsystem looks up struct_ops types from a given module BTF although it is always btf_vmlinux now. Once start using struct_ops_tab, btfs other than btf_vmlinux can be used as well.

kernel/bpf/bpf_struct_ops.c +1 -0

index e35d6321a2f8..0bc21a39257d 100644
@@ -186,6 +186,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc,
  			pr_warn("Error in init bpf_struct_ops %s\n",
  				st_ops->name);
  		} else {
+			st_ops_desc->btf = btf;
  			st_ops_desc->type_id = type_id;
  			st_ops_desc->type = t;
  			st_ops_desc->value_id = value_id;

kernel/bpf/bpf_struct_ops.c +0 -0

  {
  	unsigned int i;
 

kernel/bpf/bpf_struct_ops.c +21 -0

+static struct bpf_struct_ops_desc *
+btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
+{
+	struct btf_struct_ops_tab *tab, *new_tab;
+	int i;
+
+	if (!btf)
+		return ERR_PTR(-ENOENT);
+
+	/* Assume this function is called for a module when the module is
+	 * loading.
+	 */
+
+	tab = btf->struct_ops_tab;
+	if (!tab) {
+		tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]),
+			      GFP_KERNEL);
+		if (!tab)
+			return ERR_PTR(-ENOMEM);
+		tab->capacity = 4;
+		btf->struct_ops_tab = tab;

kernel/bpf/bpf_struct_ops.c +3 -0

+
+	return &btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++];
+}

Quoted 10 lines · On 10/30/23 12:28 PM, [email protected] wrote:

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

Maintain a registry of registered struct_ops types in the per-btf (module) struct_ops_tab. This registry allows for easy lookup of struct_ops types that are registered by a specific module.

Every struct_ops type should have an associated module BTF to provide type information since we are going to allow modules to define and register new struct_ops types. Once this change is made, the bpf_struct_ops subsystem knows where to look up type info with just a bpf_struct_ops.

I think this part needs better description. I found it hard to parse. In particular:

...
  the "bpf_struct_ops" subsystem
       knows where to look up type info with just a "bpf_struct_ops"
...

May be something like:

It is a preparation work for supporting kernel module struct_ops in a latter patch. Each struct_ops will be registered under its own kernel module btf and will be stored in the newly added btf->struct_ops_tab. The bpf verifier and bpf syscall (e.g. prog and map cmd) can find the struct_ops and its btf type/size/id... information from btf->struct_ops_tab.

Quoted 5 lines

The subsystem looks up struct_ops types from a given module BTF although it is always btf_vmlinux now. Once start using struct_ops_tab, btfs other than btf_vmlinux can be used as well.

I think this describes about the "struct btf *btf" argument change in this patch. This seems unrelated to the "add struct_ops_tab to btf" change. Can it be in its own preparation patch?

[ ... ]

Quoted 13 lines

kernel/bpf/bpf_struct_ops.c +1 -0

index e35d6321a2f8..0bc21a39257d 100644
@@ -186,6 +186,7 @@ static void bpf_struct_ops_init_one(struct 
 bpf_struct_ops_desc *st_ops_desc,
               pr_warn("Error in init bpf_struct_ops %s\n",
                   st_ops->name);
           } else {
+            st_ops_desc->btf = btf;
               st_ops_desc->type_id = type_id;
               st_ops_desc->type = t;

              st_ops_desc->value_id = value_id;

kernel/bpf/bpf_struct_ops.c void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) · line 226

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +0 -0

@@ -222,7 +223,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
 bpf_verifier_log *log)
   extern struct btf *btf_vmlinux;

  static const struct bpf_struct_ops_desc *

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)

The "!btf_vmlinux" check a few lines below should also be changed to "!btf". I think I had commented on a similar point in v5.

kernel/bpf/bpf_struct_ops.c bpf_struct_ops_find_value(u32 value_id) · line 241

Quoted 9 lines

  {
      unsigned int i;

kernel/bpf/bpf_struct_ops.c +2 -1

@@ -237,7 +238,8 @@ bpf_struct_ops_find_value(u32 value_id)
       return NULL;
   }
-const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops_desc *
+bpf_struct_ops_find(struct btf *btf, u32 type_id)

same here.

Quoted 2 lines

kernel/bpf/bpf_struct_ops.c +0 -0

   {
       unsigned int i;

[ ... ]

Quoted 25 lines

kernel/bpf/bpf_struct_ops.c +25 -0

+static struct bpf_struct_ops_desc *
+btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
+{
+    struct btf_struct_ops_tab *tab, *new_tab;
+    int i;
+
+    if (!btf)
+        return ERR_PTR(-ENOENT);
+
+    /* Assume this function is called for a module when the module is
+     * loading.
+     */
+
+    tab = btf->struct_ops_tab;
+    if (!tab) {
+        tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]),
+                  GFP_KERNEL);
+        if (!tab)
+            return ERR_PTR(-ENOMEM);
+        tab->capacity = 4;
+        btf->struct_ops_tab = tab;
+    }
+
+    for (i = 0; i < tab->cnt; i++)
+        if (tab->ops[i].st_ops == st_ops)

Quoted 6 lines

+            return ERR_PTR(-EEXIST);
+
+    if (tab->cnt == tab->capacity) {
+        new_tab = krealloc(tab, sizeof(*tab) + +                   sizeof(struct bpf_struct_ops *) * +                   tab->capacity * 2, GFP_KERNEL);

nit. Use a similar offsetof() like a few lines above.

Quoted 9 lines

+        if (!new_tab)
+            return ERR_PTR(-ENOMEM);
+        tab = new_tab;
+        tab->capacity *= 2;
+        btf->struct_ops_tab = tab;
+    }
+
+    btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;

nit. s/btf->struct_ops_tab/tab/

Quoted 3 lines

kernel/bpf/bpf_struct_ops.c +3 -0

+
+    return &btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++];
+}

kernel/bpf/bpf_struct_ops.c +20 -12

@@ -681,9 +697,17 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  	if (!st_ops_desc)
  		return ERR_PTR(-ENOTSUPP);
 
+	if (st_ops_desc->btf != btf_vmlinux) {
+		mod = btf_try_get_module(st_ops_desc->btf);
+		if (!mod)
+			return ERR_PTR(-EINVAL);
+	}
+
  	vt = st_ops_desc->value_type;
-	if (attr->value_size != vt->size)
-		return ERR_PTR(-EINVAL);
+	if (attr->value_size != vt->size) {
+		ret = -EINVAL;
+		goto errout;
+	}
 
  	t = st_ops_desc->type;
 

@@ -694,17 +718,17 @@ 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)
-		return ERR_PTR(-ENOMEM);
+	if (!st_map) {
+		ret = -ENOMEM;
+		goto errout;
+	}
 
  	st_map->st_ops_desc = st_ops_desc;
  	map = &st_map->map;
 
  	ret = bpf_jit_charge_modmem(PAGE_SIZE);
-	if (ret) {
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto errout_free;
 
  	st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
  	if (!st_map->image) {

@@ -713,23 +737,32 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  		 * here.
  		 */
  		bpf_jit_uncharge_modmem(PAGE_SIZE);
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto errout_free;
  	}
  	st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
  	st_map->links =
  		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
  				   NUMA_NO_NODE);
  	if (!st_map->uvalue || !st_map->links) {
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto errout_free;
  	}
 
  	mutex_init(&st_map->lock);

kernel/bpf/bpf_struct_ops.c +3 -0

+errout:
+	module_put(mod);
+	return ERR_PTR(ret);
  }

Quoted 64 lines · On 10/30/23 12:28 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +18 -10

@@ -681,9 +697,17 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       if (!st_ops_desc)
           return ERR_PTR(-ENOTSUPP);
+    if (st_ops_desc->btf != btf_vmlinux) {
+        mod = btf_try_get_module(st_ops_desc->btf);
+        if (!mod)
+            return ERR_PTR(-EINVAL);
+    }
+
       vt = st_ops_desc->value_type;
-    if (attr->value_size != vt->size)
-        return ERR_PTR(-EINVAL);
+    if (attr->value_size != vt->size) {
+        ret = -EINVAL;
+        goto errout;
+    }
       t = st_ops_desc->type;

@@ -694,17 +718,17 @@ 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)
-        return ERR_PTR(-ENOMEM);
+    if (!st_map) {
+        ret = -ENOMEM;
+        goto errout;
+    }
       st_map->st_ops_desc = st_ops_desc;
       map = &st_map->map;
       ret = bpf_jit_charge_modmem(PAGE_SIZE);
-    if (ret) {
-        __bpf_struct_ops_map_free(map);
-        return ERR_PTR(ret);
-    }
+    if (ret)
+        goto errout_free;
       st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
       if (!st_map->image) {

@@ -713,23 +737,32 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
            * here.
            */
           bpf_jit_uncharge_modmem(PAGE_SIZE);
-        __bpf_struct_ops_map_free(map);
-        return ERR_PTR(-ENOMEM);
+        ret = -ENOMEM;
+        goto errout_free;
       }
       st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);

      st_map->links =
          bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *), NUMA_NO_NODE);
      if (!st_map->uvalue || !st_map->links) {
-        __bpf_struct_ops_map_free(map);
-        return ERR_PTR(-ENOMEM);
+        ret = -ENOMEM;
+        goto errout_free;
      }
      mutex_init(&st_map->lock); set_vm_flush_reset_perms(st_map->image); bpf_map_init_from_attr(map, attr);
+    module_put(mod);
+

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

Quoted 5 lines

      return map;

kernel/bpf/bpf_struct_ops.c +4 -0

+
+errout_free:
+    __bpf_struct_ops_map_free(map);
+    btf = NULL;        /* has been released */

btf is not defined. I don't think this patch compiles. Something from a latter patch?

Quoted 5 lines

+errout:
+    module_put(mod);
+    return ERR_PTR(ret);
  }

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

Giving a BTF, the bpf_struct_ops knows the right place to look up type info associated with a type ID. This enables a user space program to load a struct_ops object linked to a struct_ops type defined by a module, by providing the module BTF (fd).

The bpf_prog includes attach_btf in aux which is passed along with the bpf_attr when loading the program. The purpose of attach_btf is to determine the btf type of attach_btf_id. The attach_btf_id is then used to identify the traced function for a trace program. In the case of struct_ops programs, it is used to identify the struct_ops type of the struct_ops object that a program is attached to.

kernel/bpf/bpf_struct_ops.c +1 -0

index 256516aba632..db2bbba50e38 100644
@@ -694,6 +694,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);
+	btf_put(st_map->st_ops_desc->btf);
  	bpf_map_area_free(st_map);
  }
 

@@ -735,16 +736,31 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  	const struct btf_type *t, *vt;

kernel/bpf/bpf_struct_ops.c +19 -4

+		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+		if (IS_ERR(btf))
+			return ERR_PTR(PTR_ERR(btf));
+
+		if (btf != btf_vmlinux) {
+			mod = btf_try_get_module(btf);
+			if (!mod) {
+				ret = -EINVAL;
+				goto errout;
+			}
+		}
+	} else {
+		btf = btf_vmlinux;
+		btf_get(btf);
+	}
 
-	if (st_ops_desc->btf != btf_vmlinux) {
-		mod = btf_try_get_module(st_ops_desc->btf);
-		if (!mod)
-			return ERR_PTR(-EINVAL);
+	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
+	if (!st_ops_desc) {
+		ret = -ENOTSUPP;
+		goto errout;
  	}
 
  	vt = st_ops_desc->value_type;

Quoted 6 lines · On 10/30/23 12:28 PM, [email protected] wrote:

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

Giving a BTF, the bpf_struct_ops knows the right place to look up type info associated with a type ID. This enables a user space program to load a struct_ops object linked to a struct_ops type defined by a module, by providing the module BTF (fd).

This describes about the struct_ops map creation change (by adding value_type_btf_obj_fd)? It could be described more clearly in the commit message, like specify the value_type_btf_obj_fd addition and how it is used in the struct_ops map creation.

Quoted 8 lines

The bpf_prog includes attach_btf in aux which is passed along with the bpf_attr when loading the program. The purpose of attach_btf is to determine the btf type of attach_btf_id. The attach_btf_id is then used to identify the traced function for a trace program. In the case of struct_ops programs, it is used to identify the struct_ops type of the struct_ops object that a program is attached to.

Does attach_btf_obj_fd also work?

[ ... ]

Quoted 15 lines

kernel/bpf/bpf_struct_ops.c +1 -0

index 256516aba632..db2bbba50e38 100644
@@ -694,6 +694,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);
+    btf_put(st_map->st_ops_desc->btf);
       bpf_map_area_free(st_map);
   }

@@ -735,16 +736,31 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       const struct btf_type *t, *vt;

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

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +3 -3

       struct module *mod = NULL;
       struct bpf_map *map;
+    struct btf *btf;
       int ret;
-    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
 attr->btf_vmlinux_value_type_id);
-    if (!st_ops_desc)
-        return ERR_PTR(-ENOTSUPP);
+    if (attr->value_type_btf_obj_fd) {
+        /* The map holds btf for its whole life time. */

It took me a while to parse this comment and connect it with the btf_put(st_map->st_ops_desc->btf) in the __bpf_struct_ops_map_free() above.

It is now like "btf" owns "struct_ops_desc" which also stores a pointer pointing back to itself, like "btf->struct_ops_desc->btf". The struct_ops_desc->btf was not initialized by st_map but st_map will increment its refcount much later.

Can btf be directly stored in the st_map->btf instead and map_alloc holds the refcnt of st_map->btf and btf_put(st_map->btf) in map_free?

Quoted 28 lines

kernel/bpf/bpf_struct_ops.c +19 -4

+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));
+
+        if (btf != btf_vmlinux) {
+            mod = btf_try_get_module(btf);
+            if (!mod) {
+                ret = -EINVAL;
+                goto errout;
+            }
+        }
+    } else {
+        btf = btf_vmlinux;
+        btf_get(btf);
+    }
-    if (st_ops_desc->btf != btf_vmlinux) {
-        mod = btf_try_get_module(st_ops_desc->btf);
-        if (!mod)
-            return ERR_PTR(-EINVAL);
+    st_ops_desc = bpf_struct_ops_find_value(btf, 
 attr->btf_vmlinux_value_type_id);
+    if (!st_ops_desc) {
+        ret = -ENOTSUPP;
+        goto errout;
       }
       vt = st_ops_desc->value_type;

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

Replace the static list of struct_ops types with per-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               |  36 ++++++--
  include/linux/btf.h               |   5 +-
  kernel/bpf/bpf_struct_ops.c       | 140 +++++++++---------------------
  kernel/bpf/bpf_struct_ops_types.h |  12 ---
  kernel/bpf/btf.c                  |  41 ++++++++-
  net/bpf/bpf_dummy_struct_ops.c    |  14 ++-
  net/ipv4/bpf_tcp_ca.c             |  16 +++-
  7 files changed, 140 insertions(+), 124 deletions(-)

delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

include/linux/bpf.h +31 -5

index c993df3cf699..9d7105ff06db 100644
@@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc {
  #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
  #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
  const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
  bool bpf_struct_ops_get(const void *kdata);
  void bpf_struct_ops_put(const void *kdata);
  int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,

@@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *
  {
  	return NULL;
  }
-static inline void bpf_struct_ops_init(struct btf *btf,
-				       struct bpf_verifier_log *log)
-{
-}
  static inline bool bpf_try_module_get(const void *data, struct module *owner)
  {
  	return try_module_get(owner);

@@ -3232,4 +3227,35 @@ 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);
+
+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;
+};
+
+/* 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 DEFINE_STRUCT_OPS_VALUE_TYPE(_name)			\
+extern struct bpf_struct_ops bpf_##_name;			\
+								\
+struct bpf_struct_ops_##_name {					\
+	struct bpf_struct_ops_common_value common;		\
+	struct _name data ____cacheline_aligned_in_smp;		\
+}
+
+extern int bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc,
+			       struct btf *btf,
+			       struct bpf_verifier_log *log);
+
  #endif /* _LINUX_BPF_H */

include/linux/btf.h +0 -0

index a8813605f2f6..954536431e0b 100644

include/linux/btf.h +2 -2

+		((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 */

@@ -201,6 +203,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,

@@ -575,8 +578,6 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type
  struct bpf_struct_ops;
  struct bpf_struct_ops_desc;
 
-struct bpf_struct_ops_desc *
-btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops);
  const struct bpf_struct_ops_desc *
  btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
 

kernel/bpf/bpf_struct_ops.c +0 -6

index db2bbba50e38..f3ec72be9c63 100644
@@ -13,21 +13,8 @@
  #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,
-};

kernel/bpf/bpf_struct_ops.c +0 -29

  	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_desc bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name)				\
-	[BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &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 +2 -0

+};
+

kernel/bpf/bpf_struct_ops.c +11 -0

+		return -EINVAL;
+
+	btf = btf_get_module_btf(st_ops->owner);
+	if (!btf)
+		return -EINVAL;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+	if (!log) {
+		err = -ENOMEM;
+		goto errout;
+	}

kernel/bpf/bpf_struct_ops.c +8 -0

+
+errout:
+	kfree(log);
+	btf_put(btf);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);

Quoted 83 lines · On 10/30/23 12:28 PM, [email protected] wrote:

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

Replace the static list of struct_ops types with per-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               |  36 ++++++-- include/linux/btf.h               |   5 +- kernel/bpf/bpf_struct_ops.c       | 140 +++++++++--------------------- kernel/bpf/bpf_struct_ops_types.h |  12 --- kernel/bpf/btf.c                  |  41 ++++++++- net/bpf/bpf_dummy_struct_ops.c    |  14 ++- net/ipv4/bpf_tcp_ca.c             |  16 +++-

  7 files changed, 140 insertions(+), 124 deletions(-)

  delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

include/linux/bpf.h +0 -5

index c993df3cf699..9d7105ff06db 100644
@@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc {
   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
   const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
   bool bpf_struct_ops_get(const void *kdata);
   void bpf_struct_ops_put(const void *kdata);
   int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,

@@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc 
 *bpf_struct_ops_find(struct btf *
   {
       return NULL;
   }
-static inline void bpf_struct_ops_init(struct btf *btf,
-                       struct bpf_verifier_log *log)
-{
-}
   static inline bool bpf_try_module_get(const void *data, struct module *owner)
   {

      return try_module_get(owner);

include/linux/bpf.h +31 -0

@@ -3232,4 +3227,35 @@ 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);
+
+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;
+};
+
+/* 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 DEFINE_STRUCT_OPS_VALUE_TYPE(_name)            \
+extern struct bpf_struct_ops bpf_##_name;            \
+                                \
+struct bpf_struct_ops_##_name {                    \
+    struct bpf_struct_ops_common_value common;        \
+    struct _name data ____cacheline_aligned_in_smp;        \
+}
+
+extern int bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc,
+                   struct btf *btf,
+                   struct bpf_verifier_log *log);
+
   #endif /* _LINUX_BPF_H */

include/linux/btf.h +0 -0

index a8813605f2f6..954536431e0b 100644

include/linux/btf.h line 14

Quoted 6 lines

include/linux/btf.h +1 -0

@@ -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 type *)0); is new. Why is it needed?

Quoted 38 lines

include/linux/btf.h +2 -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 */

@@ -201,6 +203,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,

@@ -575,8 +578,6 @@ static inline bool btf_type_is_struct_ptr(struct 

btf *btf, const struct btf_type
  struct bpf_struct_ops;
  struct bpf_struct_ops_desc;

include/linux/btf.h +0 -2

-struct bpf_struct_ops_desc *
-btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops);

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

kernel/bpf/bpf_struct_ops.c +0 -11

index db2bbba50e38..f3ec72be9c63 100644
@@ -13,21 +13,8 @@
   #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;
-};

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +0 -1

-#define BPF_STRUCT_OPS_COMMON_VALUE struct 

bpf_struct_ops_common_value common
-
  struct bpf_struct_ops_value {
-    BPF_STRUCT_OPS_COMMON_VALUE;
+    struct bpf_struct_ops_common_value common;

This cleanup is good. It should have been done together in patch 5 instead when refcnt and state were grouped into a new 'struct bpf_struct_ops_common_value'.

Quoted 40 lines

      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_desc bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name)                \
-    [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name },
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
   const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
   };

@@ -110,13 +68,22 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops 
 = {
   #endif

kernel/bpf/bpf_struct_ops.c const struct bpf_prog_ops bpf_struct_ops_prog_ops = { · line 73

Quoted 6 lines

kernel/bpf/bpf_struct_ops.c +3 -2

   };
-static const struct btf_type *module_type;
-static const struct btf_type *common_value_type;
+BTF_ID_LIST(st_ops_ids)
+BTF_ID(struct, module)
+BTF_ID(struct, bpf_struct_ops_common_value)

This should have been done in a separated patch immediately after patch 1. The patch 7 has unrelated changes/cleanups like this and the above BPF_STRUCT_OPS_COMMON_VALUE which could have been done earlier as preparation patches instead of packing them together with the main change here: "switch to dynamic registration". The commit message for the BTF_ID_LIST changes could be like: "A preparation to completely retire the bpf_struct_ops_init() function in the latter patch...".

kernel/bpf/bpf_struct_ops.c const struct bpf_prog_ops bpf_struct_ops_prog_ops = { · line 77

Quoted 4 lines

kernel/bpf/bpf_struct_ops.c +4 -0

+
+enum {
+    idx_module_id,
+    idx_st_ops_common_value_id,

nit. upper case to stay consistent with other similar usages.

Quoted 2 lines

kernel/bpf/bpf_struct_ops.c +2 -0

+};
+

[ ... ]

kernel/bpf/bpf_struct_ops.c

Quoted 8 lines

kernel/bpf/bpf_struct_ops.c +8 -0

+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
+{
+    struct bpf_struct_ops_desc *desc;
+    struct bpf_verifier_log *log;
+    struct btf *btf;
+    int err = 0;
+
+    if (st_ops == NULL)

NULL check is not needed. caller will never do that. If it really wanted to try, other values would have similar effect.

Quoted 16 lines

+        return -EINVAL;

kernel/bpf/bpf_struct_ops.c +14 -0

+
+    btf = btf_get_module_btf(st_ops->owner);
+    if (!btf)
+        return -EINVAL;
+
+    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+    if (!log) {
+        err = -ENOMEM;
+        goto errout;
+    }
+
+    log->level = BPF_LOG_KERNEL;
+
+    desc = btf_add_struct_ops(btf, st_ops);

Quoted 6 lines

+    if (IS_ERR(desc)) {
+        err = PTR_ERR(desc);
+        goto errout;
+    }
+
+    err = bpf_struct_ops_init(desc, btf, log);

When bpf_struct_ops_init() returns err, desc is in btf_struct_ops_tab but it is in an uninitialized state. May be do the bpf_struct_ops_init() in btf_add_struct_ops() and only increment struct_ops_tab->cnt when everything is correct.

Quoted 9 lines

kernel/bpf/bpf_struct_ops.c +8 -0

+
+errout:
+    kfree(log);
+    btf_put(btf);
+
+    return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);

Quoted 10 lines

include/linux/btf.h line 14

Quoted 8 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

Quoted 11 lines · Kui-Feng Lee

Quoted 10 lines

include/linux/btf.h line 14

Quoted 8 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

Quoted 11 lines · Kui-Feng Lee

Quoted 10 lines

include/linux/btf.h line 14

Quoted 8 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

Quoted 12 lines · Martin KaFai Lau

Quoted 11 lines · Kui-Feng Lee

Quoted 10 lines

include/linux/btf.h line 14

Quoted 8 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 6 lines

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

I mean both of them are required.
In the case of a dummy implementation, struct_ops type itself properly never being used, only being declared by the module. Without this line,

the module developer will fail to load a struct_ops map of the dummy type. This line is added to avoid this awful situation.

Quoted 22 lines · Kui-Feng Lee

Quoted 19 lines · Martin KaFai Lau

Quoted 12 lines · Kui-Feng Lee

Quoted 11 lines

include/linux/btf.h line 15

Quoted 9 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

Quoted 2 lines

I mean both of them are required.
In the case of a dummy implementation, struct_ops type itself properly never being used, only being declared by the module. Without this line,

Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be used somewhere in the kernel or module. Like tcp must be using the tcp_congestion_ops after reg(). bpf_dummy_ops is very special and probably should be moved out to bpf_testmod somehow but this is for later. Even bpf_dummy_ops does not have an issue now. Why it is needed after the kmod support change?

or it is a preemptive addition to be future proof only?

Addition is fine if it is required to work. I am trying to understand why this new addition is needed after the kmod support change. The reason why this is needed after the kmod support change is not obvious from looking at the code. The commit message didn't mention why and what broke after this kmod change. If someone wants to clean it up a few months later, we will need to figure out why it was added in the first place.

Quoted 5 lines

the module developer will fail to load a struct_ops map of the dummy type. This line is added to avoid this awful situation.

Quoted 27 lines · Martin KaFai Lau

Quoted 15 lines · Kui-Feng Lee

Quoted 12 lines · Martin KaFai Lau

Quoted 11 lines · Kui-Feng Lee

Quoted 10 lines

include/linux/btf.h line 14

Quoted 8 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

I mean both of them are required.
In the case of a dummy implementation, struct_ops type itself properly never being used, only being declared by the module. Without this line,

Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be used somewhere in the kernel or module. Like tcp must be using the tcp_congestion_ops after reg(). bpf_dummy_ops is very special and probably should be moved out to bpf_testmod somehow but this is for later. Even bpf_dummy_ops does not have an issue now. Why it is needed after the kmod support change?

Quoted 3 lines

or it is a preemptive addition to be future proof only?

Addition is fine if it is required to work. I am trying to understand why this new addition is needed after the kmod support change. The reason why this is needed after the kmod support change is not obvious from looking at the code. The commit message didn't mention why and what broke after this kmod change. If someone wants to clean it up a few months later, we will need to figure out why it was added in the first place.

It is a future proof.
What do you think if I add a comment in the code?

Quoted 7 lines

Quoted 6 lines

the module developer will fail to load a struct_ops map of the dummy type. This line is added to avoid this awful situation.

Quoted 33 lines · Kui-Feng Lee

Quoted 30 lines · Martin KaFai Lau

Quoted 16 lines · Kui-Feng Lee

Quoted 13 lines · Martin KaFai Lau

Quoted 12 lines · Kui-Feng Lee

Quoted 11 lines

include/linux/btf.h line 15

Quoted 9 lines

include/linux/btf.h +1 -0

index a8813605f2f6..954536431e0b 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 type *)0); is new. Why is it needed?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

This is a trick of BTF to force compiler generate type info for the given type. Without trick, compiler may skip these types if these type are not used at all in the module.  For example, modules usually don't use value types of struct_ops directly.

It is not the value type and value type emit is understood. It is the struct_ops type itself and it is new addition in this patchset afaict. The value type emit is in the next line which was cut out from the context here.

I mean both of them are required.
In the case of a dummy implementation, struct_ops type itself properly never being used, only being declared by the module. Without this line,

Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be used somewhere in the kernel or module. Like tcp must be using the tcp_congestion_ops after reg(). bpf_dummy_ops is very special and probably should be moved out to bpf_testmod somehow but this is for later. Even bpf_dummy_ops does not have an issue now. Why it is needed after the kmod support change?

or it is a preemptive addition to be future proof only?

Quoted 5 lines

Quoted 1 line

Addition is fine if it is required to work. I am trying to understand why this new addition is needed after the kmod support change. The reason why this is needed after the kmod support change is not obvious from looking at the code. The commit message didn't mention why and what broke after this kmod change. If someone wants to clean it up a few months later, we will need to figure out why it was added in the first place.

It is a future proof.
What do you think if I add a comment in the code?

If it is not required to work, I prefer not adding it to avoid confusion and avoid future cleanup temptation. Even the artificial bpf_dummy_ops does not need it, so not enough reason to introduce this code redundancy.

Switch topic.
While we are on a new macro topic, I think a new macro will be useful to emit the value type and register_bpf_struct_ops together. wdyt?

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

the module developer will fail to load a struct_ops map of the dummy type. This line is added to avoid this awful situation.

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +27 -0

new file mode 100644
index 000000000000..3a00dc294583
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.h"
+
+static void test_regular_load(void)
+{
+	struct struct_ops_module *skel;
+	struct bpf_link *link;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	int err;
+
+	skel = struct_ops_module__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+		return;
+	err = struct_ops_module__load(skel);
+	if (!ASSERT_OK(err, "struct_ops_module_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+	ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+	/* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
+	ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +32 -0

+{
+	struct rcu_tasks_trace_gp *rcu;
+	time_t start;
+	long gp_seq;
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+	rcu = rcu_tasks_trace_gp__open_and_load();
+	if (IS_ERR(rcu))
+		return -EFAULT;
+	if (rcu_tasks_trace_gp__attach(rcu))
+		return -EFAULT;
+
+	gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+	if (bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace),
+				   &opts))
+		return -EFAULT;
+	if (opts.retval != 0)
+		return -EFAULT;
+
+	start = time(NULL);
+	while ((start + 2) > time(NULL) &&
+	       gp_seq == READ_ONCE(rcu->bss->gp_seq))
+		sched_yield();
+
+	rcu_tasks_trace_gp__destroy(rcu);
+
+	return 0;
+}
+
  /*
   * Trigger synchronize_rcu() in kernel.
   */
  int kern_sync_rcu(void)
  {
+	if (kern_sync_rcu_tasks_trace())
+		return -EFAULT;
  	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
  }

Quoted 33 lines · On 10/30/23 12:28 PM, [email protected] wrote:

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +27 -0

new file mode 100644
index 000000000000..3a00dc294583
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.h"
+
+static void test_regular_load(void)
+{
+    struct struct_ops_module *skel;
+    struct bpf_link *link;
+    DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+    int err;
+
+    skel = struct_ops_module__open_opts(&opts);
+    if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+        return;
+    err = struct_ops_module__load(skel);
+    if (!ASSERT_OK(err, "struct_ops_module_load"))
+        return;
+
+    link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+    ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+    /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
+    ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c line 37

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +10 -0

+
+    bpf_link__destroy(link);
+
+    struct_ops_module__destroy(skel);
+}
+
+void serial_test_struct_ops_module(void)
+{
+    if (test__start_subtest("regular_load"))
+        test_regular_load();

Could it also add some negative tests, e.g. missing 'struct bpf_struct_ops_common_value', reg() when the module is gone...etc.

[ ... ]

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c

Quoted 2 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +2 -0

+/* This function will trigger call_rcu_tasks_trace() in the kernel */
+static int kern_sync_rcu_tasks_trace(void)

With patch 4, is it still needed?

Quoted 41 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +15 -0

+{
+    struct rcu_tasks_trace_gp *rcu;
+    time_t start;
+    long gp_seq;
+    LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+    rcu = rcu_tasks_trace_gp__open_and_load();
+    if (IS_ERR(rcu))
+        return -EFAULT;
+    if (rcu_tasks_trace_gp__attach(rcu))
+        return -EFAULT;
+
+    gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+    if 

(bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace), +                   &opts))
+        return -EFAULT;
+    if (opts.retval != 0)
+        return -EFAULT;

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +11 -0

+
+    start = time(NULL);
+    while ((start + 2) > time(NULL) &&
+           gp_seq == READ_ONCE(rcu->bss->gp_seq))
+        sched_yield();
+
+    rcu_tasks_trace_gp__destroy(rcu);
+
+    return 0;
+}
+

  /*
   * Trigger synchronize_rcu() in kernel.
   */
  int kern_sync_rcu(void)
  {
+    if (kern_sync_rcu_tasks_trace())
+        return -EFAULT;
      return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0); }

Quoted 49 lines · Martin KaFai Lau

Quoted 33 lines · On 10/30/23 12:28 PM, [email protected] wrote:

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +27 -0

new file mode 100644
index 000000000000..3a00dc294583
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.h"
+
+static void test_regular_load(void)
+{
+    struct struct_ops_module *skel;
+    struct bpf_link *link;
+    DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+    int err;
+
+    skel = struct_ops_module__open_opts(&opts);
+    if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+        return;
+    err = struct_ops_module__load(skel);
+    if (!ASSERT_OK(err, "struct_ops_module_load"))
+        return;
+
+    link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+    ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+    /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
+    ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c line 37

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +10 -0

+
+    bpf_link__destroy(link);
+
+    struct_ops_module__destroy(skel);
+}
+
+void serial_test_struct_ops_module(void)
+{
+    if (test__start_subtest("regular_load"))
+        test_regular_load();

Could it also add some negative tests, e.g. missing 'struct bpf_struct_ops_common_value', reg() when the module is gone...etc.

[ ... ]

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c line 39

Quoted 2 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +2 -0

+/* This function will trigger call_rcu_tasks_trace() in the kernel */
+static int kern_sync_rcu_tasks_trace(void)

With patch 4, is it still needed?

Patch 4 shortens time of holding the module, but it still can happen since bpf_link_put() is performed asynchronously.

Quoted 43 lines

Quoted 42 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +15 -0

+{
+    struct rcu_tasks_trace_gp *rcu;
+    time_t start;
+    long gp_seq;
+    LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+    rcu = rcu_tasks_trace_gp__open_and_load();
+    if (IS_ERR(rcu))
+        return -EFAULT;
+    if (rcu_tasks_trace_gp__attach(rcu))
+        return -EFAULT;
+
+    gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+    if 

(bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace), +                   &opts))
+        return -EFAULT;
+    if (opts.retval != 0)
+        return -EFAULT;

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +11 -0

+
+    start = time(NULL);
+    while ((start + 2) > time(NULL) &&
+           gp_seq == READ_ONCE(rcu->bss->gp_seq))
+        sched_yield();
+
+    rcu_tasks_trace_gp__destroy(rcu);
+
+    return 0;
+}
+

  /*
   * Trigger synchronize_rcu() in kernel.
   */
  int kern_sync_rcu(void)
  {
+    if (kern_sync_rcu_tasks_trace())
+        return -EFAULT;
      return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0); }

Quoted 58 lines · Kui-Feng Lee

Quoted 54 lines · Martin KaFai Lau

Quoted 34 lines · On 10/30/23 12:28 PM, [email protected] wrote:

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +26 -0

new file mode 100644
index 000000000000..3a00dc294583
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.h"
+
+static void test_regular_load(void)
+{
+    struct struct_ops_module *skel;
+    struct bpf_link *link;
+    DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+    int err;
+
+    skel = struct_ops_module__open_opts(&opts);
+    if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+        return;
+    err = struct_ops_module__load(skel);
+    if (!ASSERT_OK(err, "struct_ops_module_load"))
+        return;
+
+    link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+    ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+    /* test_2() will be called from bpf_dummy_reg() in 

bpf_testmod.c */
+    ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c line 37

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +10 -0

+
+    bpf_link__destroy(link);
+
+    struct_ops_module__destroy(skel);
+}
+
+void serial_test_struct_ops_module(void)
+{
+    if (test__start_subtest("regular_load"))
+        test_regular_load();

Could it also add some negative tests, e.g. missing 'struct bpf_struct_ops_common_value', reg() when the module is gone...etc.

[ ... ]

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c

Quoted 2 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +2 -0

+/* This function will trigger call_rcu_tasks_trace() in the kernel */
+static int kern_sync_rcu_tasks_trace(void)

With patch 4, is it still needed?

Quoted 2 lines

Patch 4 shortens time of holding the module, but it still can happen since bpf_link_put() is performed asynchronously.

Is the link pinned to a file that triggers bpf_link_put()? Otherwise, close() should reach bpf_link_put_direct() which is synchronous.

Even if it went through bpf_link_put(), rcu_tasks_trace_gp is very specific to the bpf sleepable tracing prog. Is it the correct one to wait?

Quoted 44 lines

Quoted 44 lines

Quoted 43 lines

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +15 -0

+{
+    struct rcu_tasks_trace_gp *rcu;
+    time_t start;
+    long gp_seq;
+    LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+    rcu = rcu_tasks_trace_gp__open_and_load();
+    if (IS_ERR(rcu))
+        return -EFAULT;
+    if (rcu_tasks_trace_gp__attach(rcu))
+        return -EFAULT;
+
+    gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+    if 

(bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace), +                   &opts))
+        return -EFAULT;
+    if (opts.retval != 0)
+        return -EFAULT;

tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c new file +11 -0

+
+    start = time(NULL);
+    while ((start + 2) > time(NULL) &&
+           gp_seq == READ_ONCE(rcu->bss->gp_seq))
+        sched_yield();
+
+    rcu_tasks_trace_gp__destroy(rcu);
+
+    return 0;
+}
+

  /*
   * Trigger synchronize_rcu() in kernel.
   */
  int kern_sync_rcu(void)
  {
+    if (kern_sync_rcu_tasks_trace())
+        return -EFAULT;
      return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0); }

Changes from v5:

- As the 2nd patch, we introduce "bpf_struct_ops_desc". This change involves moving certain members of "bpf_struct_ops" to "bpf_struct_ops_desc", which becomes a part of "btf_struct_ops_tab". This ensures that these members remain accessible even when the owner module of a "bpf_struct_ops" is unloaded.

- Correct the order of arguments when calling
in the 3rd patch.

- Remove the owner argument from bpf_struct_ops_init_one(). Instead,

    callers should fill in st_ops->owner.

- Make sure to hold the owner module when calling bpf_struct_ops_find() and bpf_struct_ops_find_value() in the 6th patch.

- Merge the functions register_bpf_struct_ops_btf() and register_bpf_struct_ops() into a single function and relocate it to btf.c for better organization and clarity.

- Undo the name modifications made to find_kernel_btf_id() and

Quoted 19 lines · On 10/30/23 12:28 PM, [email protected] wrote:

Changes from v5:

  - As the 2nd patch, we introduce "bpf_struct_ops_desc". This change involves moving certain members of "bpf_struct_ops" to "bpf_struct_ops_desc", which becomes a part of "btf_struct_ops_tab". This ensures that these members remain accessible even when the owner module of a "bpf_struct_ops" is unloaded.

  - Correct the order of arguments when calling
     in the 3rd patch.

  - Remove the owner argument from bpf_struct_ops_init_one(). Instead, callers should fill in st_ops->owner.

  - Make sure to hold the owner module when calling bpf_struct_ops_find() and bpf_struct_ops_find_value() in the 6th patch.

Quoted 5 lines

  - Merge the functions register_bpf_struct_ops_btf() and register_bpf_struct_ops() into a single function and relocate it to btf.c for better organization and clarity.

  - Undo the name modifications made to find_kernel_btf_id() and

The find_kernel_attach_btf_id name change is still in patch 8. tbh, I don't have a strong preference on the name here. Could it be in a separate patch for naming cleanup or at least mention it in the commit message?