Registrating struct_ops types from modules

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

Get ready to remove bpf_struct_ops_init() in the future. By using BPF_ID_LIST, it is possible to gather type information while building

kernel/bpf/bpf_struct_ops.c +2 -0

+BTF_ID_LIST(st_ops_ids)
+BTF_ID(struct, module)

net/bpf/bpf_dummy_struct_ops.c +2 -0

index 2748f9d77b18..bd753dbccaf6 100644
@@ -17,6 +17,8 @@ struct bpf_dummy_ops_test_args {
  	struct bpf_dummy_ops_state state;
  };
 
+static struct btf *bpf_dummy_ops_btf;
+
  static struct bpf_dummy_ops_test_args *
  dummy_ops_init_args(const union bpf_attr *kattr, unsigned int nr)
  {

net/bpf/bpf_dummy_struct_ops.c +2 -0

+	if (prog->aux->attach_btf_id != type_id)
  		return -EOPNOTSUPP;
 
  	func_proto = prog->aux->attach_func_proto;

@@ -142,6 +148,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 
  static int bpf_dummy_init(struct btf *btf)
  {
+	bpf_dummy_ops_btf = btf;
  	return 0;
  }
 

net/ipv4/bpf_tcp_ca.c +7 -1

index c7bbd8f3c708..5bb56c9ad4e5 100644
@@ -20,6 +20,7 @@ static u32 unsupported_ops[] = {
 
  static const struct btf_type *tcp_sock_type;
  static u32 tcp_sock_id, sock_id;
+static const struct btf_type *tcp_congestion_ops_type;
 
  static int bpf_tcp_ca_init(struct btf *btf)
  {

@@ -36,6 +37,11 @@ static int bpf_tcp_ca_init(struct btf *btf)
  	tcp_sock_id = type_id;
  	tcp_sock_type = btf_type_by_id(btf, tcp_sock_id);
 
+	type_id = btf_find_by_name_kind(btf, "tcp_congestion_ops", BTF_KIND_STRUCT);
+	if (type_id < 0)
+		return -EINVAL;
+	tcp_congestion_ops_type = btf_type_by_id(btf, type_id);
+
  	return 0;
  }
 

@@ -149,7 +155,7 @@ static u32 prog_ops_moff(const struct bpf_prog *prog)
  	u32 midx;
 
  	midx = prog->expected_attach_type;
-	t = bpf_tcp_congestion_ops.type;
+	t = tcp_congestion_ops_type;
  	m = &btf_type_member(t)[midx];
 
  	return __btf_member_bit_offset(t, m) / 8;

kernel/bpf/btf.c +5 -0

+		return NULL;
+
+	*ret_cnt = btf->struct_ops_tab->cnt;
+	return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
+}

Quoted 6 lines · On 12/8/23 4:26 PM, [email protected] wrote:

+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)

        *ret_cnt = 0;

unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.

The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen.

Quoted 6 lines

+        return NULL;

kernel/bpf/btf.c +4 -0

+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
+}

Quoted 13 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:26 PM, [email protected] wrote:

+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)

         *ret_cnt = 0;

unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.

Quoted 1 line

The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen.

Checking callers, I didn't find anything that make btf here NULL so far.

It is safe to remove !btf check. For the same reason as assigning *ret_cnt for safe, this check should be fine here as well, right?

I don't have strong opinion here. What I though is to keep the values as it is without any side-effect if the function call fails and if possible. And, the callers should not expect the callee to set some specific values when a call fails.

Quoted 8 lines

Quoted 7 lines

+        return NULL;

kernel/bpf/btf.c +4 -0

+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
+}

Quoted 16 lines · Kui-Feng Lee

Quoted 13 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:26 PM, [email protected] wrote:

+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)

         *ret_cnt = 0;

unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.

Quoted 6 lines

Quoted 1 line

The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen.

Checking callers, I didn't find anything that make btf here NULL so far.

It is safe to remove !btf check. For the same reason as assigning *ret_cnt for safe, this check should be fine here as well, right?

If for safety, why ref_cnt is not checked for NULL also? The userspace passed-in btf should have been checked for NULL long time before reaching here. There is no need to be over protective here. It would really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON though).

afaict, no function in btf.c is checking the btf argument for NULL also.

Quoted 5 lines

I don't have strong opinion here. What I though is to keep the values as it is without any side-effect if the function call fails and if possible. And, the callers should not expect the callee to set some specific values when a call fails.

For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good signal on how this function will be used.

I think it is arguable whether returning NULL here is failure. I would argue getting a 0 struct_ops_desc array is not a failure. It is why the !btf case confuses the return NULL case to mean a never would happen error instead of meaning there is no struct_ops. Taking out the !btf case, NULL means there is no struct_ops (instead of failure), so 0 cnt.

Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt won't be set when there is no struct_ops in the btf.

When looking at it again, how about moving the bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the new btf_get_struct_ops() function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

+        return NULL;
+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops_desc
*)btf->struct_ops_tab->ops;
+}

Quoted 34 lines · Martin KaFai Lau

Quoted 16 lines · Kui-Feng Lee

Quoted 13 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:26 PM, [email protected] wrote:

+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)

         *ret_cnt = 0;

unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.

Quoted 6 lines

Quoted 1 line

The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen.

Checking callers, I didn't find anything that make btf here NULL so far.

It is safe to remove !btf check. For the same reason as assigning *ret_cnt for safe, this check should be fine here as well, right?

If for safety, why ref_cnt is not checked for NULL also? The userspace passed-in btf should have been checked for NULL long time before reaching here. There is no need to be over protective here. It would really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON though).

afaict, no function in btf.c is checking the btf argument for NULL also.

Quoted 5 lines

I don't have strong opinion here. What I though is to keep the values as it is without any side-effect if the function call fails and if possible. And, the callers should not expect the callee to set some specific values when a call fails.

Quoted 5 lines

For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good signal on how this function will be used.

I think it is arguable whether returning NULL here is failure. I would argue getting a 0 struct_ops_desc array is not a failure. It is why the !btf case confuses the return NULL case to mean a never would happen error instead of meaning there is no struct_ops. Taking out the !btf case, NULL means there is no struct_ops (instead of failure), so 0 cnt.

Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt won't be set when there is no struct_ops in the btf.

I will fix at the patch 10 by setting local cnt 0.

Quoted 1 line

Quoted 2 lines

When looking at it again, how about moving the bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the new btf_get_struct_ops() function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.

I prefer to keep them in bpf_struct_ops.c if it is ok to you. Fixing the initialization issue of bpf_struct_ops_find() should be enough.

Quoted 11 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

+        return NULL;

kernel/bpf/btf.c +4 -0

+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
+}

Quoted 47 lines · Kui-Feng Lee

Quoted 34 lines · Martin KaFai Lau

Quoted 16 lines · Kui-Feng Lee

Quoted 13 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:26 PM, [email protected] wrote:

+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+    if (!btf)
+        return NULL;
+    if (!btf->struct_ops_tab)

         *ret_cnt = 0;

unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.

Quoted 6 lines

Quoted 1 line

The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen.

Checking callers, I didn't find anything that make btf here NULL so far.

It is safe to remove !btf check. For the same reason as assigning *ret_cnt for safe, this check should be fine here as well, right?

If for safety, why ref_cnt is not checked for NULL also? The userspace passed-in btf should have been checked for NULL long time before reaching here. There is no need to be over protective here. It would really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON though).

afaict, no function in btf.c is checking the btf argument for NULL also.

Quoted 5 lines

I don't have strong opinion here. What I though is to keep the values as it is without any side-effect if the function call fails and if possible. And, the callers should not expect the callee to set some specific values when a call fails.

Quoted 5 lines

For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good signal on how this function will be used.

I think it is arguable whether returning NULL here is failure. I would argue getting a 0 struct_ops_desc array is not a failure. It is why the !btf case confuses the return NULL case to mean a never would happen error instead of meaning there is no struct_ops. Taking out the !btf case, NULL means there is no struct_ops (instead of failure), so 0 cnt.

Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt won't be set when there is no struct_ops in the btf.

I will fix at the patch 10 by setting local cnt 0

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

When looking at it again, how about moving the bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the new btf_get_struct_ops() function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.

I prefer to keep them in bpf_struct_ops.c if it is ok to you. Fixing the initialization issue of bpf_struct_ops_find() should be enough.

If choosing between fixing the bug in patch 10 and moving them to btf.c, I would avoid patch 10 (and future) bug to begin with by moving them to btf.c. Moving them should be cheap unless there is other dependency that I have overlooked.

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

+        return NULL;
+
+    *ret_cnt = btf->struct_ops_tab->cnt;
+    return (const struct bpf_struct_ops_desc
*)btf->struct_ops_tab->ops;
+}

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  	struct bpf_struct_ops_map *st_map;
  	const struct btf_type *t, *vt;
  	struct bpf_map *map;
+	struct btf *btf;

kernel/bpf/bpf_struct_ops.c +11 -2

+	}
+
+	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;
-	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;
 

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

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

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

            return ERR_CAST(btf);

It needs to check for btf_is_module:

        if (!btf_is_module(btf)) {
            btf_put(btf);
            return ERR_PTR(-EINVAL);
        }

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

Quoted 4 lines

kernel/bpf/bpf_struct_ops.c +3 -0

+    } else {
+        btf = btf_vmlinux;
+        btf_get(btf);

btf_vmlinux could be NULL or a ERR_PTR? Lets take this chance to use bpf_get_btf_vmlinux():
        btf = bpf_get_btf_vmlinux();
            if (IS_ERR(btf))
                    return ERR_CAST(btf);

Going back to patch 4. This should be the only case that btf will be NULL or ERR_PTR?

will continue the remaining review later tonight.

pw-bot: cr

Quoted 16 lines

kernel/bpf/bpf_struct_ops.c +11 -2

+    }
+
+    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;
-    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;

Quoted 20 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union 
 bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

kernel/bpf/bpf_struct_ops.c static struct bpf_map *bpf_struct_ops_map_alloc(union · line 693

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

             return ERR_CAST(btf);

Quoted 6 lines

It needs to check for btf_is_module:

         if (!btf_is_module(btf)) {
             btf_put(btf);
             return ERR_PTR(-EINVAL);
         }

Even btf is btf_vmlinux the kernel's btf, it still works.

Although libbpf pass 0 as the value of value_type_btf_obj_fd for btf_vmlinux now, it should be OK for a user space loader to pass a fd of btf_vmlinux.

WDYT?

Quoted 25 lines · Kui-Feng Lee

Quoted 22 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

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

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

             return ERR_CAST(btf);

It needs to check for btf_is_module:

Quoted 6 lines

Quoted 4 lines

         if (!btf_is_module(btf)) {
             btf_put(btf);
             return ERR_PTR(-EINVAL);
         }

Even btf is btf_vmlinux the kernel's btf, it still works.

btf could be a bpf program's btf. It needs to ensure it is a kernel module btf here.

Quoted 6 lines

Although libbpf pass 0 as the value of value_type_btf_obj_fd for btf_vmlinux now, it should be OK for a user space loader to pass a fd of btf_vmlinux.

WDYT?

Quoted 31 lines · Martin KaFai Lau

Quoted 30 lines · Kui-Feng Lee

Quoted 26 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

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

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

             return ERR_CAST(btf);

It needs to check for btf_is_module:

         if (!btf_is_module(btf)) {
             btf_put(btf);
             return ERR_PTR(-EINVAL);
         }

Quoted 3 lines

Quoted 1 line

Even btf is btf_vmlinux the kernel's btf, it still works.

btf could be a bpf program's btf. It needs to ensure it is a kernel module btf here.

Got it!

Quoted 8 lines

Quoted 7 lines

Although libbpf pass 0 as the value of value_type_btf_obj_fd for btf_vmlinux now, it should be OK for a user space loader to pass a fd of btf_vmlinux.

WDYT?

Quoted 34 lines · Kui-Feng Lee

Quoted 31 lines · Martin KaFai Lau

Quoted 30 lines · Kui-Feng Lee

Quoted 26 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

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

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

             return ERR_CAST(btf);

It needs to check for btf_is_module:

         if (!btf_is_module(btf)) {
             btf_put(btf);
             return ERR_PTR(-EINVAL);
         }

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Even btf is btf_vmlinux the kernel's btf, it still works.

btf could be a bpf program's btf. It needs to ensure it is a kernel module btf here.

Got it!

Isn't btf_is_kernel() better here?
User space may pass a fd to btf_vmlinux.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Although libbpf pass 0 as the value of value_type_btf_obj_fd for btf_vmlinux now, it should be OK for a user space loader to pass a fd of btf_vmlinux.

WDYT?

Quoted 41 lines · Kui-Feng Lee

Quoted 38 lines · Kui-Feng Lee

Quoted 34 lines · Martin KaFai Lau

Quoted 23 lines · Kui-Feng Lee

Quoted 20 lines · Martin KaFai Lau

Quoted 6 lines · On 12/8/23 4:27 PM, [email protected] wrote:

(quoted diff) +1 -0

@@ -681,15 +682,30 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
       struct bpf_map *map;
+    struct btf *btf;

Review static struct bpf_map · line 693

Quoted 10 lines

(quoted diff) +5 -3

       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. */
+        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+        if (IS_ERR(btf))
+            return ERR_PTR(PTR_ERR(btf));

             return ERR_CAST(btf);

Quoted 8 lines

Quoted 6 lines

It needs to check for btf_is_module:

         if (!btf_is_module(btf)) {
             btf_put(btf);
             return ERR_PTR(-EINVAL);
         }

Even btf is btf_vmlinux the kernel's btf, it still works.

btf could be a bpf program's btf. It needs to ensure it is a kernel module btf here.

Quoted 4 lines

Quoted 1 line

Got it!

Isn't btf_is_kernel() better here?
User space may pass a fd to btf_vmlinux.

Limit it to btf_is_module. What is the benefit of supporting btf_vmlinux as a fd while fd 0 already means btf_vmlinux?

kfunc does not support the btf_vmlinux as fd also, supporting in struct_ops alone is confusing. I don't think the major user (libbpf) does this either, so really not much usage other than a confusing API to have both fd == 0 and a btf_vmlinux's fd to mean btf_vmlinux.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Although libbpf pass 0 as the value of value_type_btf_obj_fd for btf_vmlinux now, it should be OK for a user space loader to pass a fd of btf_vmlinux.

WDYT?

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.

struct bpf_strct_ops_map doesn't hold a refcnt to btf anymore sicne a

module will hold a refcnt to it's btf already. But, struct_ops programs are different. They hold their associated btf, not the module since they need only btf to assure their types (signatures).

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

---
  include/linux/bpf.h          |  1 +
  include/linux/bpf_verifier.h |  1 +
  kernel/bpf/bpf_struct_ops.c  | 28 +++++++++++++++++++++++-----
  kernel/bpf/verifier.c        | 10 ++++++++++
  4 files changed, 35 insertions(+), 5 deletions(-)

include/linux/bpf.h +1 -0

index 91bcd62d6fcf..c5c7cc4552f5 100644
@@ -1681,6 +1681,7 @@ struct bpf_struct_ops {
  	void (*unreg)(void *kdata);
  	int (*update)(void *kdata, void *old_kdata);
  	int (*validate)(void *kdata);
+	struct module *owner;
  	const char *name;
  	struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
  };

include/linux/bpf_verifier.h +1 -0

index 314b679fb494..01113bcdd479 100644
@@ -651,6 +651,7 @@ struct bpf_verifier_env {
  	u32 prev_insn_idx;
  	struct bpf_prog *prog;		/* eBPF program being verified */
  	const struct bpf_verifier_ops *ops;
+	struct module *attach_btf_mod;	/* The owner module of prog->aux->attach_btf */
  	struct bpf_verifier_stack_elem *head; /* stack of verifier states to be processed */
  	int stack_size;			/* number of states to be processed */
  	bool strict_alignment;		/* perform strict pointer alignment checks */

kernel/bpf/bpf_struct_ops.c +0 -0

index f943f8378e76..a838f7c7d583 100644
@@ -641,12 +641,15 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
  		bpf_jit_uncharge_modmem(PAGE_SIZE);
  	}

kernel/bpf/bpf_struct_ops.c +16 -4

+
  	/* The struct_ops's function may switch to another struct_ops.
  	 *
  	 * For example, bpf_tcp_cc_x->init() may switch to

@@ -681,6 +684,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  	size_t st_map_size;
  	struct bpf_struct_ops_map *st_map;
  	const struct btf_type *t, *vt;
+	struct module *mod = NULL;
  	struct bpf_map *map;
  	struct btf *btf;
  	int ret;

@@ -690,10 +694,20 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
  		if (IS_ERR(btf))
  			return ERR_PTR(PTR_ERR(btf));
-	} else {
+
+		if (btf != btf_vmlinux) {
+			mod = btf_try_get_module(btf);
+			if (!mod) {
+				btf_put(btf);
+				return ERR_PTR(-EINVAL);
+			}
+		}
+		/* mod (NULL for btf_vmlinux) holds a refcnt to btf. We
+		 * don't need an extra refcnt here.
+		 */
+		btf_put(btf);
+	} else
  		btf = btf_vmlinux;
-		btf_get(btf);
-	}
 
  	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
  	if (!st_ops_desc) {

@@ -756,7 +770,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
  errout_free:
  	__bpf_struct_ops_map_free(map);
  errout:
-	btf_put(btf);
+	module_put(mod);
 
  	return ERR_PTR(ret);
  }

kernel/bpf/verifier.c +9 -0

+		/* Make sure st_ops is valid through the lifetime of env */
+		env->attach_btf_mod = btf_try_get_module(btf);
+		if (!env->attach_btf_mod) {
+			verbose(env, "owner module of btf is not found\n");
+			return -ENOTSUPP;
+		}
+	}
 
  	btf_id = prog->aux->attach_btf_id;
  	st_ops_desc = bpf_struct_ops_find(btf, btf_id);

@@ -20792,6 +20800,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
  		env->prog->expected_attach_type = 0;
 
  	*prog = env->prog;
+
+	module_put(env->attach_btf_mod);
  err_unlock:
  	if (!is_priv)
  		mutex_unlock(&bpf_verifier_lock);

Quoted 6 lines · On 12/8/23 4:27 PM, [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.

struct bpf_strct_ops_map doesn't hold a refcnt to btf anymore sicne a

s /bpf_strct_/bpf_struct_/

s/sicne/since/

Quoted 3 lines

module will hold a refcnt to it's btf already. But, struct_ops programs are different. They hold their associated btf, not the module since they need only btf to assure their types (signatures).

The patch subject is not accurate. The patch holds the module refcnt when verifying the bpf prog also. May be "hold module refcnt in struct_ops map creation and prog verification".

The commit message also is inaccurate on the prog load. It did not mention the module is also held when loading struct_ops prog but it is only held during the verification time. Please explain why it is only needed during the verification time.

Quoted 42 lines

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

---
  include/linux/bpf.h          |  1 + include/linux/bpf_verifier.h |  1 + kernel/bpf/bpf_struct_ops.c  | 28 +++++++++++++++++++++++----- kernel/bpf/verifier.c        | 10 ++++++++++

  4 files changed, 35 insertions(+), 5 deletions(-)

include/linux/bpf.h +1 -0

index 91bcd62d6fcf..c5c7cc4552f5 100644
@@ -1681,6 +1681,7 @@ struct bpf_struct_ops {
       void (*unreg)(void *kdata);
       int (*update)(void *kdata, void *old_kdata);
       int (*validate)(void *kdata);
+    struct module *owner;
       const char *name;
       struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
   };

include/linux/bpf_verifier.h +0 -0

index 314b679fb494..01113bcdd479 100644
@@ -651,6 +651,7 @@ struct bpf_verifier_env {
       u32 prev_insn_idx;

      struct bpf_prog *prog;        /* eBPF program being verified */
      const struct bpf_verifier_ops *ops;
+    struct module *attach_btf_mod;    /* The owner module of prog->aux->attach_btf */
      struct bpf_verifier_stack_elem *head; /* stack of verifier states to be processed */ int stack_size;            /* number of states to be processed */ bool strict_alignment;        /* perform strict pointer alignment checks */

kernel/bpf/bpf_struct_ops.c +0 -0

index f943f8378e76..a838f7c7d583 100644
@@ -641,12 +641,15 @@ static void __bpf_struct_ops_map_free(struct 
 bpf_map *map)
           bpf_jit_uncharge_modmem(PAGE_SIZE);
       }

kernel/bpf/bpf_struct_ops.c static void __bpf_struct_ops_map_free(struct · line 652

Quoted 10 lines

kernel/bpf/bpf_struct_ops.c +3 -1

       bpf_map_area_free(st_map->uvalue);
-    btf_put(st_map->btf);
       bpf_map_area_free(st_map);
   }
   static void bpf_struct_ops_map_free(struct bpf_map *map)
   {
+    struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map 
 *)map;
+
+    module_put(st_map->st_ops_desc->st_ops->owner);

The module_get was not done on st_ops->owner when st_map->btf is btf_vmlinux (i.e. not module). Although it probably does not matter, I would feel more comfortable if it only releases for the things that it did acquire earlier.

    /* st_ops->owner was acquired during map_alloc to implicitly holds
     * the btf's refcnt. The acquire was only done when btf_is_module()
     * st_map->btf cannot be NULL here.
     */
    if (btf_is_module(st_map->btf)) module_put(st_map->st_ops_desc->st_ops->owner);

Quoted 50 lines

kernel/bpf/bpf_struct_ops.c +1 -0

+
       /* The struct_ops's function may switch to another struct_ops.
        *

       * For example, bpf_tcp_cc_x->init() may switch to

kernel/bpf/bpf_struct_ops.c +1 -0

@@ -681,6 +684,7 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
       size_t st_map_size;
       struct bpf_struct_ops_map *st_map;
       const struct btf_type *t, *vt;
+    struct module *mod = NULL;
       struct bpf_map *map;
       struct btf *btf;

      int ret;

kernel/bpf/bpf_struct_ops.c +13 -3

@@ -690,10 +694,20 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)
           btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
           if (IS_ERR(btf))
               return ERR_PTR(PTR_ERR(btf));
-    } else {
+
+        if (btf != btf_vmlinux) {
+            mod = btf_try_get_module(btf);
+            if (!mod) {
+                btf_put(btf);
+                return ERR_PTR(-EINVAL);
+            }
+        }
+        /* mod (NULL for btf_vmlinux) holds a refcnt to btf. We
+         * don't need an extra refcnt here.
+         */
+        btf_put(btf);
+    } else
           btf = btf_vmlinux;
-        btf_get(btf);
-    }
       st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
       if (!st_ops_desc) {

@@ -756,7 +770,7 @@ static struct bpf_map 
 *bpf_struct_ops_map_alloc(union bpf_attr *attr)

  errout_free:
      __bpf_struct_ops_map_free(map);
  errout:
-    btf_put(btf);
+    module_put(mod);
      return ERR_PTR(ret);
  }

kernel/bpf/bpf_struct_ops.c +0 -0

@@ -886,6 +900,10 @@ static int bpf_struct_ops_map_link_update(struct 

bpf_link *link, struct bpf_map
      if (!bpf_struct_ops_valid_to_reg(new_map))

Quoted 5 lines

          return -EINVAL;
+    /* The old map is holding the refcount for the owner module.  The +     * ownership of the owner module refcount is going to be +     * transferred from the old map to the new map.
+     */

This part I don't understand. Both old and new map hold its own module's refcount at map_alloc time and release its own module refcnt during map_free(). Where the module refcount transfer happened?

kernel/bpf/verifier.c static int check_struct_ops_btf_id(struct · line 20082

Quoted 12 lines

      if (!st_map->st_ops_desc->st_ops->update)
          return -EOPNOTSUPP;

kernel/bpf/verifier.c +1 -0

index 795c16f9cf57..c303cf2fb5ff 100644
@@ -20079,6 +20079,14 @@ static int check_struct_ops_btf_id(struct 
 bpf_verifier_env *env)
       }
       btf = prog->aux->attach_btf;
+    if (btf != btf_vmlinux) {

    if (btf_is_module(btf)) {

Quoted 18 lines

kernel/bpf/verifier.c +7 -0

+        /* Make sure st_ops is valid through the lifetime of env */
+        env->attach_btf_mod = btf_try_get_module(btf);
+        if (!env->attach_btf_mod) {
+            verbose(env, "owner module of btf is not found\n");
+            return -ENOTSUPP;
+        }
+    }
       btf_id = prog->aux->attach_btf_id;
       st_ops_desc = bpf_struct_ops_find(btf, btf_id);

@@ -20792,6 +20800,8 @@ int bpf_check(struct bpf_prog **prog, union 

bpf_attr *attr, bpfptr_t uattr, __u3
          env->prog->expected_attach_type = 0;
      *prog = env->prog;
+
+    module_put(env->attach_btf_mod);
  err_unlock:
      if (!is_priv)
          mutex_unlock(&bpf_verifier_lock);

include/linux/bpf.h +1 -0

index c5c7cc4552f5..7384806ee74e 100644
@@ -3321,4 +3321,16 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
  	return prog->aux->func_idx != 0;
  }
 
+enum bpf_struct_ops_state {

Quoted 13 lines · On 12/8/23 4:27 PM, [email protected] wrote:

include/linux/bpf.h +5 -0

index c5c7cc4552f5..7384806ee74e 100644
@@ -3321,4 +3321,16 @@ static inline bool bpf_is_subprog(const struct 
 bpf_prog *prog)
       return prog->aux->func_idx != 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,

include/linux/bpf.h static inline bool bpf_is_subprog(const struct bpf_prog *prog) · line 3334

Quoted 6 lines

include/linux/bpf.h +6 -0

+};
+
+struct bpf_struct_ops_common_value {
+    refcount_t refcnt;
+    enum bpf_struct_ops_state state;
+};

nit. Move these up closer to the existing 'struct bpf_struct_ops' and related functions. Probably under 'struct bpf_struct_ops'.

include/linux/bpf.h +0 -5

index 7384806ee74e..c881befa35f5 100644
@@ -1698,7 +1698,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,

@@ -1744,10 +1743,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 +8 -0

+	register_bpf_struct_ops(st_ops);      \
+})
+
  enum bpf_struct_ops_state {
  	BPF_STRUCT_OPS_STATE_INIT,
  	BPF_STRUCT_OPS_STATE_INUSE,

@@ -3333,4 +3336,19 @@ struct bpf_struct_ops_common_value {
  	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.
+ *
+ */

include/linux/btf.h +0 -0

  #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
 
  /* These need to be macros, as the expressions are used in assembler input */

  static const struct bpf_struct_ops_desc *
  bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
  {

include/linux/btf.h +7 -4

+	const struct bpf_struct_ops_desc *st_ops_list;
  	unsigned int i;
+	u32 cnt = 0;
 
-	if (!value_id || !btf)
+	if (!value_id)
  		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)
+			return &st_ops_list[i];
  	}
 
  	return NULL;

include/linux/btf.h +2 -0

+		if (st_ops_list[i].type_id == type_id)
+			return &st_ops_list[i];
  	}
 
  	return NULL;

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

deleted file mode 100644
index 5678a9ddf817..000000000000
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* internal file - do not include directly */
-
-#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

kernel/bpf/btf.c +1 -2

index edbe3cbf2dcc..5545dee3ff54 100644
@@ -19,6 +19,7 @@
  #include <linux/bpf_verifier.h>
  #include <linux/btf.h>
  #include <linux/btf_ids.h>
+#include <linux/bpf.h>
  #include <linux/bpf_lsm.h>
  #include <linux/skmsg.h>
  #include <linux/perf_event.h>

@@ -5792,8 +5793,6 @@ struct btf *btf_parse_vmlinux(void)
  	/* btf_parse_vmlinux() runs under bpf_verifier_lock */
  	bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
 
-	bpf_struct_ops_init(btf, log);
-
  	refcount_set(&btf->refcnt, 1);
 
  	err = btf_alloc_id(btf);

@@ -8621,11 +8620,21 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
  	return !strncmp(reg_name, arg_name, cmp_len);

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

new file mode 100644
index 000000000000..55a4c6ed92aa
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "struct_ops_module.skel.h"
+#include "testmod_unload.skel.h"
+
+static void test_regular_load(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	struct struct_ops_module *skel;
+	struct testmod_unload *skel_unload;
+	struct bpf_link *link_map_free = NULL;
+	struct bpf_link *link;
+	int err, i;
+
+	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"))
+		goto cleanup;
+
+	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");
+
+	bpf_link__destroy(link);
+
+cleanup:
+	skel_unload = testmod_unload__open_and_load();
+
+	if (ASSERT_OK_PTR(skel_unload, "testmod_unload_open"))
+		link_map_free = bpf_program__attach(skel_unload->progs.trace_map_free);
+	struct_ops_module__destroy(skel);
+
+	if (!ASSERT_OK_PTR(link_map_free, "create_link_map_free"))
+		return;
+
+	/* Wait for the struct_ops map to be freed. Struct_ops maps hold a
+	 * refcount to the module btf. And, this function unloads and then
+	 * loads bpf_testmod. Without waiting the map to be freed, the next
+	 * test may fail to unload the bpf_testmod module since the map is
+	 * still holding a refcnt to the module.
+	 */
+	for (i = 0; i < 10; i++) {
+		if (skel_unload->bss->bpf_testmod_put)
+			break;
+		usleep(100000);
+	}
+	ASSERT_EQ(skel_unload->bss->bpf_testmod_put, 1, "map_free");
+
+	bpf_link__destroy(link_map_free);
+	testmod_unload__destroy(skel_unload);
+}
+
+static void test_load_without_module(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	struct struct_ops_module *skel;

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

+	ASSERT_ERR(err, "struct_ops_module_load");
+
+	struct_ops_module__destroy(skel);
+
+cleanup:
+	/* Without this, the next test may fail */
+	load_bpf_testmod(false);
+}
+
+void serial_test_struct_ops_module(void)
+{
+	if (test__start_subtest("regular_load"))
+		test_regular_load();
+
+	if (test__start_subtest("load_without_module"))
+		test_load_without_module();
+}
+

Quoted 76 lines · On 12/8/23 4:27 PM, [email protected] wrote:

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

new file mode 100644
index 000000000000..55a4c6ed92aa
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "struct_ops_module.skel.h"
+#include "testmod_unload.skel.h"
+
+static void test_regular_load(void)
+{
+    DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+    struct struct_ops_module *skel;
+    struct testmod_unload *skel_unload;
+    struct bpf_link *link_map_free = NULL;
+    struct bpf_link *link;
+    int err, i;
+
+    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"))
+        goto cleanup;
+
+    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");
+
+    bpf_link__destroy(link);
+
+cleanup:
+    skel_unload = testmod_unload__open_and_load();
+
+    if (ASSERT_OK_PTR(skel_unload, "testmod_unload_open"))
+        link_map_free = 

bpf_program__attach(skel_unload->progs.trace_map_free); +    struct_ops_module__destroy(skel);

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

+
+    if (!ASSERT_OK_PTR(link_map_free, "create_link_map_free"))
+        return;
+
+    /* Wait for the struct_ops map to be freed. Struct_ops maps hold a
+     * refcount to the module btf. And, this function unloads and then
+     * loads bpf_testmod. Without waiting the map to be freed, the next
+     * test may fail to unload the bpf_testmod module since the map is
+     * still holding a refcnt to the module.
+     */
+    for (i = 0; i < 10; i++) {
+        if (skel_unload->bss->bpf_testmod_put)
+            break;
+        usleep(100000);
+    }
+    ASSERT_EQ(skel_unload->bss->bpf_testmod_put, 1, "map_free");
+
+    bpf_link__destroy(link_map_free);
+    testmod_unload__destroy(skel_unload);
+}
+
+static void test_load_without_module(void)
+{
+    DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+    struct struct_ops_module *skel;
+    int err;
+
+    err = unload_bpf_testmod(false);
+    if (!ASSERT_OK(err, "unload_bpf_testmod"))
+        return;

Quoted 5 lines

+
+    skel = struct_ops_module__open_opts(&opts); +    if (!ASSERT_OK_PTR(skel, "struct_ops_module_open")) +        goto cleanup;
+    err = struct_ops_module__load(skel);

Both the module btf and the .ko itself are gone from the kernel now?
This is basically testing libbpf cannot find 'struct bpf_testmod_ops' from the running kernel?

How about create another struct_ops_module_notfound.c bpf program:
SEC(".struct_ops.link")
struct bpf_testmod_ops_notfound testmod_1 = {
    .test_1 = (void *)test_1,
    .test_2 = (void *)test_2,
};

and avoid the usleep() wait and the unload_bpf_testmod()?

Quoted 19 lines

+    ASSERT_ERR(err, "struct_ops_module_load");

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

+
+    struct_ops_module__destroy(skel);
+
+cleanup:
+    /* Without this, the next test may fail */
+    load_bpf_testmod(false);
+}
+
+void serial_test_struct_ops_module(void)
+{
+    if (test__start_subtest("regular_load"))
+        test_regular_load();
+
+    if (test__start_subtest("load_without_module"))
+        test_load_without_module();
+}
+

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

Include btf object id (btf_obj_id) in bpf_map_info so that tools (ex:
bpftools struct_ops dump) know the correct btf from the kernel to look up type information of struct_ops types.

Since struct_ops types can be defined and registered in a module. The type information of a struct_ops type are defined in the btf of the module defining it. The userspace tools need to know which btf is for the module defining a struct_ops type.

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

---
  include/linux/bpf.h            | 1 +
  include/uapi/linux/bpf.h       | 2 +-
  kernel/bpf/bpf_struct_ops.c    | 7 +++++++
  kernel/bpf/syscall.c           | 2 ++
  tools/include/uapi/linux/bpf.h | 2 +-
  5 files changed, 12 insertions(+), 2 deletions(-)

include/linux/bpf.h +0 -0

index c881befa35f5..26103d8a4374 100644

include/uapi/linux/bpf.h +0 -0

  	__u64 map_extra;
  } __attribute__((aligned(8)));
 

kernel/bpf/bpf_struct_ops.c +7 -0

index fd26716fa0f9..51c0de75aa85 100644
@@ -979,3 +979,10 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
  	kfree(link);
  	return err;
  }
+
+void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map)
+{
+	struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
+
+	info->btf_obj_id = btf_obj_id(st_map->btf);
+}

kernel/bpf/syscall.c +0 -0

index 4aced7e58904..3cab56cd02ff 100644

kernel/bpf/syscall.c +0 -0

  	if (bpf_map_is_offloaded(map)) {
  		err = bpf_map_offload_info_fill(&info, map);

tools/include/uapi/linux/bpf.h +1 -1

index 5c3838a97554..716c6b28764d 100644
@@ -6534,7 +6534,7 @@ struct bpf_map_info {
  	__u32 btf_id;
  	__u32 btf_key_type_id;
  	__u32 btf_value_type_id;
-	__u32 :32;	/* alignment pad */
+	__u32 btf_obj_id;
  	__u64 map_extra;
  } __attribute__((aligned(8)));
 

Quoted 20 lines · On 12/8/23 4:27 PM, [email protected] wrote:

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

Include btf object id (btf_obj_id) in bpf_map_info so that tools (ex:
bpftools struct_ops dump) know the correct btf from the kernel to look up type information of struct_ops types.

Since struct_ops types can be defined and registered in a module. The type information of a struct_ops type are defined in the btf of the module defining it.  The userspace tools need to know which btf is for the module defining a struct_ops type.

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

---
  include/linux/bpf.h            | 1 +
  include/uapi/linux/bpf.h       | 2 +-
  kernel/bpf/bpf_struct_ops.c    | 7 +++++++
  kernel/bpf/syscall.c           | 2 ++
  tools/include/uapi/linux/bpf.h | 2 +-

  5 files changed, 12 insertions(+), 2 deletions(-)

include/linux/bpf.h struct bpf_struct_ops_##_name · line 3355

Quoted 11 lines

include/linux/bpf.h +1 -0

index c881befa35f5..26103d8a4374 100644
@@ -3350,5 +3350,6 @@ struct bpf_struct_ops_##_name 
 {                    \
   int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
                    struct btf *btf,
                    struct bpf_verifier_log *log);
+void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct 
 bpf_map *map);

This needs to be in the CONFIG_BPF_JIT guard also.

include/uapi/linux/bpf.h struct bpf_map_info { · line 6537

Quoted 12 lines

  #endif /* _LINUX_BPF_H */

include/uapi/linux/bpf.h +1 -1

index 5c3838a97554..716c6b28764d 100644
@@ -6534,7 +6534,7 @@ struct bpf_map_info {
       __u32 btf_id;
       __u32 btf_key_type_id;
       __u32 btf_value_type_id;
-    __u32 :32;    /* alignment pad */
+    __u32 btf_obj_id;

may be "btf_vmlinux_id" to make it clear it is a kernel btf and should be used with map_info->btf_vmlinux_value_type_id.

Quoted 25 lines

include/uapi/linux/bpf.h +0 -0

       __u64 map_extra;
   } __attribute__((aligned(8)));

kernel/bpf/bpf_struct_ops.c +2 -0

index fd26716fa0f9..51c0de75aa85 100644
@@ -979,3 +979,10 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
       kfree(link);
       return err;
   }
+
+void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct 

bpf_map *map)
+{
+    struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;

kernel/bpf/bpf_struct_ops.c +3 -0

+
+    info->btf_obj_id = btf_obj_id(st_map->btf);
+}

kernel/bpf/syscall.c +0 -0

index 4aced7e58904..3cab56cd02ff 100644
@@ -4715,6 +4715,8 @@ static int bpf_map_get_info_by_fd(struct file 

kernel/bpf/syscall.c static int bpf_map_get_info_by_fd(struct file *file, · line 4719

Quoted 6 lines

kernel/bpf/syscall.c +2 -0

 *file,
           info.btf_value_type_id = map->btf_value_type_id;
       }
       info.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
+    if (map->map_type == BPF_MAP_TYPE_STRUCT_OPS)
+        bpf_map_struct_ops_info_fill(&info, map);

This patch should be moved earlier in the set instead of after the selftest patch 13. May be after patch 5 where st_map->btf is added.

and where is the test for this?

Quoted 15 lines

kernel/bpf/syscall.c +0 -0

       if (bpf_map_is_offloaded(map)) {
           err = bpf_map_offload_info_fill(&info, map);

tools/include/uapi/linux/bpf.h +1 -1

index 5c3838a97554..716c6b28764d 100644
@@ -6534,7 +6534,7 @@ struct bpf_map_info {
       __u32 btf_id;
       __u32 btf_key_type_id;
       __u32 btf_value_type_id;
-    __u32 :32;    /* alignment pad */
+    __u32 btf_obj_id;
       __u64 map_extra;
   } __attribute__((aligned(8)));