diff mbox series

[bpf-next,1/2] libbpf: Skip zeroed or null fields if not found in the kernel type.

Message ID 20240312183245.341141-2-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Ignore additional fields in the struct_ops maps in an updated version. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
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-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
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-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
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-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-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-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / 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-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-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
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 8 maintainers not CCed: daniel@iogearbox.net kpsingh@kernel.org haoluo@google.com sdf@google.com yonghong.song@linux.dev jolsa@kernel.org john.fastabend@gmail.com eddyz87@gmail.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, 56 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-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs 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-8 fail 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-23 fail 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-22 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 fail 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-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
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-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-39 fail 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-38 fail 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-40 fail 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-32 fail 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-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-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-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc

Commit Message

Kui-Feng Lee March 12, 2024, 6:32 p.m. UTC 
Accept additional fields of a struct_ops type with all zero values even if
these fields are not in the corresponding type in the kernel. This provides
a way to be backward compatible. User space programs can use the same map
on a machine running an old kernel by clearing fields that do not exist in
the kernel.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

Comments

Andrii Nakryiko March 12, 2024, 11:01 p.m. UTC | #1 
On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> Accept additional fields of a struct_ops type with all zero values even if
> these fields are not in the corresponding type in the kernel. This provides
> a way to be backward compatible. User space programs can use the same map
> on a machine running an old kernel by clearing fields that do not exist in
> the kernel.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index efab29b8935b..715879796046 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                 __u32 kern_member_idx;
>                 const char *mname;
>
> +               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);

you don't need to move this up here, btf__resolve_size() can skip
modifiers and typedefs just fine, so let's keep this one below and
just pass member->type to btf__resolve_size()

>                 mname = btf__name_by_offset(btf, member->name_off);
> +               moff = member->offset / 8;
> +               mdata = data + moff;
> +               msize = btf__resolve_size(btf, mtype_id);
> +               if (msize < 0) {
> +                       pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",

s/fails/failed/

> +                               map->name, mname);
> +                       return msize;
> +               }
> +
>                 kern_member = find_member_by_name(kern_btf, kern_type, mname);
>                 if (!kern_member) {
>                         pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
>                                 map->name, mname);
> +
> +                       /* Skip all zeros or null fields if they are not
> +                        * presented in the kernel BTF.
> +                        */
> +                       if (btf_is_ptr(mtype)) {
> +                               if (!st_ops->progs[i])
> +                                       continue;

so, this is both unnecessary to check for NULL (libbpf_is_mem_zeroed
will do it just fine for pointers as well), but it's also wrong
because user could have set this program pointer through skeleton's
shadow type, while here you won't yet see. So let's drop this
btf_is_ptr() special case, and just do generic libbpf_is_mem_zeroed()
check

> +                       } else {
> +                               if (libbpf_is_mem_zeroed(mdata, msize))
> +                                       continue;

I think it's worth emitting informational message here, something like

pr_info("struct_ops %s: member %s not found in kernel, skipping it as
it's set to zero\n", ...")

?

and move that pr_warn("Cannot find member %s in kernel BTF") after
this check, so we don't have scary-looking error-level message

> +                       }
> +
>                         return -ENOTSUP;
>                 }
>
> @@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                         return -ENOTSUP;
>                 }
>
> -               moff = member->offset / 8;
>                 kern_moff = kern_member->offset / 8;
> -
> -               mdata = data + moff;
>                 kern_mdata = kern_data + kern_moff;
> -
> -               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
>                 kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
>                                                     &kern_mtype_id);
>                 if (BTF_INFO_KIND(mtype->info) !=
> @@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                         continue;
>                 }
>
> -               msize = btf__resolve_size(btf, mtype_id);
>                 kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
> -               if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
> +               if (kern_msize < 0 || msize != kern_msize) {
>                         pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
>                                 map->name, mname, (ssize_t)msize,
>                                 (ssize_t)kern_msize);
> --
> 2.34.1
>
Kui-Feng Lee March 13, 2024, 12:22 a.m. UTC | #2 
On 3/12/24 16:01, Andrii Nakryiko wrote:
> On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> Accept additional fields of a struct_ops type with all zero values even if
>> these fields are not in the corresponding type in the kernel. This provides
>> a way to be backward compatible. User space programs can use the same map
>> on a machine running an old kernel by clearing fields that do not exist in
>> the kernel.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index efab29b8935b..715879796046 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                  __u32 kern_member_idx;
>>                  const char *mname;
>>
>> +               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
> 
> you don't need to move this up here, btf__resolve_size() can skip
> modifiers and typedefs just fine, so let's keep this one below and
> just pass member->type to btf__resolve_size()

Ok!

> 
>>                  mname = btf__name_by_offset(btf, member->name_off);
>> +               moff = member->offset / 8;
>> +               mdata = data + moff;
>> +               msize = btf__resolve_size(btf, mtype_id);
>> +               if (msize < 0) {
>> +                       pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",
> 
> s/fails/failed/
> 
>> +                               map->name, mname);
>> +                       return msize;
>> +               }
>> +
>>                  kern_member = find_member_by_name(kern_btf, kern_type, mname);
>>                  if (!kern_member) {
>>                          pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
>>                                  map->name, mname);
>> +
>> +                       /* Skip all zeros or null fields if they are not
>> +                        * presented in the kernel BTF.
>> +                        */
>> +                       if (btf_is_ptr(mtype)) {
>> +                               if (!st_ops->progs[i])
>> +                                       continue;
> 
> so, this is both unnecessary to check for NULL (libbpf_is_mem_zeroed
> will do it just fine for pointers as well), but it's also wrong
> because user could have set this program pointer through skeleton's
> shadow type, while here you won't yet see. So let's drop this
> btf_is_ptr() special case, and just do generic libbpf_is_mem_zeroed()
> check

You are right! I will fix it.

> 
>> +                       } else {
>> +                               if (libbpf_is_mem_zeroed(mdata, msize))
>> +                                       continue;
> 
> I think it's worth emitting informational message here, something like
> 
> pr_info("struct_ops %s: member %s not found in kernel, skipping it as
> it's set to zero\n", ...")
> 
> ?
> 
> and move that pr_warn("Cannot find member %s in kernel BTF") after
> this check, so we don't have scary-looking error-level message

Sure!

> 
>> +                       }
>> +
>>                          return -ENOTSUP;
>>                  }
>>
>> @@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          return -ENOTSUP;
>>                  }
>>
>> -               moff = member->offset / 8;
>>                  kern_moff = kern_member->offset / 8;
>> -
>> -               mdata = data + moff;
>>                  kern_mdata = kern_data + kern_moff;
>> -
>> -               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
>>                  kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
>>                                                      &kern_mtype_id);
>>                  if (BTF_INFO_KIND(mtype->info) !=
>> @@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          continue;
>>                  }
>>
>> -               msize = btf__resolve_size(btf, mtype_id);
>>                  kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
>> -               if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
>> +               if (kern_msize < 0 || msize != kern_msize) {
>>                          pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
>>                                  map->name, mname, (ssize_t)msize,
>>                                  (ssize_t)kern_msize);
>> --
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index efab29b8935b..715879796046 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1131,11 +1131,33 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		__u32 kern_member_idx;
 		const char *mname;
 
+		mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
 		mname = btf__name_by_offset(btf, member->name_off);
+		moff = member->offset / 8;
+		mdata = data + moff;
+		msize = btf__resolve_size(btf, mtype_id);
+		if (msize < 0) {
+			pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",
+				map->name, mname);
+			return msize;
+		}
+
 		kern_member = find_member_by_name(kern_btf, kern_type, mname);
 		if (!kern_member) {
 			pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
 				map->name, mname);
+
+			/* Skip all zeros or null fields if they are not
+			 * presented in the kernel BTF.
+			 */
+			if (btf_is_ptr(mtype)) {
+				if (!st_ops->progs[i])
+					continue;
+			} else {
+				if (libbpf_is_mem_zeroed(mdata, msize))
+					continue;
+			}
+
 			return -ENOTSUP;
 		}
 
@@ -1147,13 +1169,8 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 			return -ENOTSUP;
 		}
 
-		moff = member->offset / 8;
 		kern_moff = kern_member->offset / 8;
-
-		mdata = data + moff;
 		kern_mdata = kern_data + kern_moff;
-
-		mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
 		kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
 						    &kern_mtype_id);
 		if (BTF_INFO_KIND(mtype->info) !=
@@ -1230,9 +1247,8 @@  static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 			continue;
 		}
 
-		msize = btf__resolve_size(btf, mtype_id);
 		kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
-		if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
+		if (kern_msize < 0 || msize != kern_msize) {
 			pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
 				map->name, mname, (ssize_t)msize,
 				(ssize_t)kern_msize);