diff mbox series

[bpf-next,v1,1/8] libbpf: allow version suffixes (___smth) for struct_ops types

Message ID 20240227204556.17524-2-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series libbpf: type suffixes and autocreate flag for struct_ops maps | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers warning 6 maintainers not CCed: jolsa@kernel.org john.fastabend@gmail.com kpsingh@kernel.org song@kernel.org sdf@google.com haoluo@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 59 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Eduard Zingerman Feb. 27, 2024, 8:45 p.m. UTC 
E.g. allow the following struct_ops definitions:

    struct bpf_testmod_ops___v1 { int (*test)(void); };
    struct bpf_testmod_ops___v2 { int (*test)(void); };

    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v1 a = { .test = ... }
    SEC(".struct_ops.link")
    struct bpf_testmod_ops___v2 b = { .test = ... }

Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
resolved as 'struct bpf_testmod_ops' from kernel BTF.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Kui-Feng Lee Feb. 27, 2024, 9:47 p.m. UTC | #1 
On 2/27/24 12:45, Eduard Zingerman wrote:
> E.g. allow the following struct_ops definitions:
> 
>      struct bpf_testmod_ops___v1 { int (*test)(void); };
>      struct bpf_testmod_ops___v2 { int (*test)(void); };
> 
>      SEC(".struct_ops.link")
>      struct bpf_testmod_ops___v1 a = { .test = ... }
>      SEC(".struct_ops.link")
>      struct bpf_testmod_ops___v2 b = { .test = ... }
> 
> Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
> resolved as 'struct bpf_testmod_ops' from kernel BTF.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 01f407591a92..abe663927013 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>   				   const char *name, __u32 kind);
>   
>   static int
> -find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>   			   struct module_btf **mod_btf,
>   			   const struct btf_type **type, __u32 *type_id,
>   			   const struct btf_type **vtype, __u32 *vtype_id,
> @@ -957,15 +957,21 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	const struct btf_type *kern_type, *kern_vtype;
>   	const struct btf_member *kern_data_member;
>   	struct btf *btf;
> -	__s32 kern_vtype_id, kern_type_id;
> +	__s32 kern_vtype_id, kern_type_id, err;
> +	char *tname;
>   	__u32 i;
>   
> +	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
> +	if (!tname)
> +		return -ENOMEM;
> +
>   	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
>   					&btf, mod_btf);
>   	if (kern_type_id < 0) {
>   		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>   			tname);
> -		return kern_type_id;
> +		err = kern_type_id;
> +		goto err_out;
>   	}
>   	kern_type = btf__type_by_id(btf, kern_type_id);
>   
> @@ -979,7 +985,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	if (kern_vtype_id < 0) {
>   		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
>   			STRUCT_OPS_VALUE_PREFIX, tname);
> -		return kern_vtype_id;
> +		err = kern_vtype_id;
> +		goto err_out;
>   	}
>   	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
>   
> @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	if (i == btf_vlen(kern_vtype)) {
>   		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
>   			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto err_out;
>   	}
>   
>   	*type = kern_type;
> @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>   	*data_member = kern_data_member;

Where is going to free tname when it successes?

>   
>   	return 0;
> +
> +err_out:
> +	free(tname);
> +	return err;
>   }
>   
>   static bool bpf_map__is_struct_ops(const struct bpf_map *map)
Eduard Zingerman Feb. 27, 2024, 9:49 p.m. UTC | #2 
On Tue, 2024-02-27 at 13:47 -0800, Kui-Feng Lee wrote:
[...]

> > @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> >   	if (i == btf_vlen(kern_vtype)) {
> >   		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
> >   			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> > -		return -EINVAL;
> > +		err = -EINVAL;
> > +		goto err_out;
> >   	}
> >   
> >   	*type = kern_type;
> > @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> >   	*data_member = kern_data_member;
> 
> Where is going to free tname when it successes?

My bad, thank you for spotting this.

> >   
> >   	return 0;
> > +
> > +err_out:
> > +	free(tname);
> > +	return err;
> >   }
> >   
> >   static bool bpf_map__is_struct_ops(const struct bpf_map *map)
David Vernet Feb. 28, 2024, 4:29 p.m. UTC | #3 
On Tue, Feb 27, 2024 at 10:45:49PM +0200, Eduard Zingerman wrote:
> E.g. allow the following struct_ops definitions:
> 
>     struct bpf_testmod_ops___v1 { int (*test)(void); };
>     struct bpf_testmod_ops___v2 { int (*test)(void); };
> 
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v1 a = { .test = ... }
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops___v2 b = { .test = ... }
> 
> Where both bpf_testmod_ops__v1 and bpf_testmod_ops__v2 would be
> resolved as 'struct bpf_testmod_ops' from kernel BTF.
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

Acked-by: David Vernet <void@manifault.com>

Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
we could just do this on the stack, but I guess there's no static max size for
a tname.

> ---
>  tools/lib/bpf/libbpf.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 01f407591a92..abe663927013 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -948,7 +948,7 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>  				   const char *name, __u32 kind);
>  
>  static int
> -find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>  			   struct module_btf **mod_btf,
>  			   const struct btf_type **type, __u32 *type_id,
>  			   const struct btf_type **vtype, __u32 *vtype_id,
> @@ -957,15 +957,21 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	const struct btf_type *kern_type, *kern_vtype;
>  	const struct btf_member *kern_data_member;
>  	struct btf *btf;
> -	__s32 kern_vtype_id, kern_type_id;
> +	__s32 kern_vtype_id, kern_type_id, err;
> +	char *tname;
>  	__u32 i;
>  
> +	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
> +	if (!tname)
> +		return -ENOMEM;
> +
>  	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
>  					&btf, mod_btf);
>  	if (kern_type_id < 0) {
>  		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>  			tname);
> -		return kern_type_id;
> +		err = kern_type_id;
> +		goto err_out;
>  	}
>  	kern_type = btf__type_by_id(btf, kern_type_id);
>  
> @@ -979,7 +985,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	if (kern_vtype_id < 0) {
>  		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
>  			STRUCT_OPS_VALUE_PREFIX, tname);
> -		return kern_vtype_id;
> +		err = kern_vtype_id;
> +		goto err_out;
>  	}
>  	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
>  
> @@ -997,7 +1004,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	if (i == btf_vlen(kern_vtype)) {
>  		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
>  			tname, STRUCT_OPS_VALUE_PREFIX, tname);
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto err_out;
>  	}
>  
>  	*type = kern_type;
> @@ -1007,6 +1015,10 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>  	*data_member = kern_data_member;
>  
>  	return 0;
> +
> +err_out:
> +	free(tname);
> +	return err;
>  }
>  
>  static bool bpf_map__is_struct_ops(const struct bpf_map *map)
> -- 
> 2.43.0
>
Eduard Zingerman Feb. 28, 2024, 5:28 p.m. UTC | #4 
On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
[...]

> Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> we could just do this on the stack, but I guess there's no static max size for
> a tname.

GCC documents [0] that it does not impose name length limits.
Skimming through libbpf's btf.c it looks like it does not impose limits either.
I can add a name buffer and a fallback to strdup logic if tname is too long,
but I don't think this code would ever be on the hot path.

[0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation
David Vernet Feb. 28, 2024, 5:30 p.m. UTC | #5 
On Wed, Feb 28, 2024 at 07:28:49PM +0200, Eduard Zingerman wrote:
> On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
> [...]
> 
> > Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> > we could just do this on the stack, but I guess there's no static max size for
> > a tname.
> 
> GCC documents [0] that it does not impose name length limits.
> Skimming through libbpf's btf.c it looks like it does not impose limits either.
> I can add a name buffer and a fallback to strdup logic if tname is too long,
> but I don't think this code would ever be on the hot path.
> 
> [0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation

Yeah, definitely not worth it. Thanks for looking at the documentation just in
case.
Andrii Nakryiko Feb. 28, 2024, 11:21 p.m. UTC | #6 
On Wed, Feb 28, 2024 at 9:29 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-02-28 at 10:29 -0600, David Vernet wrote:
> [...]
>
> > Modulo the leak pointed out by Kui-Feng in another thread. It would be nice if
> > we could just do this on the stack, but I guess there's no static max size for
> > a tname.
>
> GCC documents [0] that it does not impose name length limits.
> Skimming through libbpf's btf.c it looks like it does not impose limits either.
> I can add a name buffer and a fallback to strdup logic if tname is too long,
> but I don't think this code would ever be on the hot path.

It would still be nice to avoid allocation even if for the sake of
simplifying error handling. I think it's reasonable to have `char
name[256]` on the stack and snprintf() into it. Let's keep it simple.

>
> [0] https://gcc.gnu.org/onlinedocs/gcc/Identifiers-implementation.html#Identifiers-implementation
Eduard Zingerman Feb. 28, 2024, 11:37 p.m. UTC | #7 
On Wed, 2024-02-28 at 15:21 -0800, Andrii Nakryiko wrote:

[...]

> It would still be nice to avoid allocation even if for the sake of
> simplifying error handling. I think it's reasonable to have `char
> name[256]` on the stack and snprintf() into it. Let's keep it simple.

Ok
>
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 01f407591a92..abe663927013 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -948,7 +948,7 @@  static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
 				   const char *name, __u32 kind);
 
 static int
-find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
+find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
 			   struct module_btf **mod_btf,
 			   const struct btf_type **type, __u32 *type_id,
 			   const struct btf_type **vtype, __u32 *vtype_id,
@@ -957,15 +957,21 @@  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	const struct btf_type *kern_type, *kern_vtype;
 	const struct btf_member *kern_data_member;
 	struct btf *btf;
-	__s32 kern_vtype_id, kern_type_id;
+	__s32 kern_vtype_id, kern_type_id, err;
+	char *tname;
 	__u32 i;
 
+	tname = strndup(tname_raw, bpf_core_essential_name_len(tname_raw));
+	if (!tname)
+		return -ENOMEM;
+
 	kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
 					&btf, mod_btf);
 	if (kern_type_id < 0) {
 		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
 			tname);
-		return kern_type_id;
+		err = kern_type_id;
+		goto err_out;
 	}
 	kern_type = btf__type_by_id(btf, kern_type_id);
 
@@ -979,7 +985,8 @@  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	if (kern_vtype_id < 0) {
 		pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
 			STRUCT_OPS_VALUE_PREFIX, tname);
-		return kern_vtype_id;
+		err = kern_vtype_id;
+		goto err_out;
 	}
 	kern_vtype = btf__type_by_id(btf, kern_vtype_id);
 
@@ -997,7 +1004,8 @@  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	if (i == btf_vlen(kern_vtype)) {
 		pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
 			tname, STRUCT_OPS_VALUE_PREFIX, tname);
-		return -EINVAL;
+		err = -EINVAL;
+		goto err_out;
 	}
 
 	*type = kern_type;
@@ -1007,6 +1015,10 @@  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
 	*data_member = kern_data_member;
 
 	return 0;
+
+err_out:
+	free(tname);
+	return err;
 }
 
 static bool bpf_map__is_struct_ops(const struct bpf_map *map)