diff mbox series

[v2,bpf-next] libbpf: make remark about zero-initializing bpf_*_info structs

Message ID ZcxsEQ8Ld_hqbi7L@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [v2,bpf-next] libbpf: make remark about zero-initializing bpf_*_info structs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-46 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-44 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-43 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-47 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-45 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-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-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-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs 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-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-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-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
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-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-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-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-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
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-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-34 success Logs for x86_64-llvm-17 / veristat
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-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-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-35 success Logs for x86_64-llvm-18 / build / build for 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-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-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-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier 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
netdev/series_format success Single patches do not need cover letters
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 10 maintainers not CCed: jolsa@kernel.org daniel@iogearbox.net john.fastabend@gmail.com yonghong.song@linux.dev martin.lau@linux.dev song@kernel.org sdf@google.com eddyz87@gmail.com kpsingh@kernel.org 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, 49 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 24 this patch: 24
netdev/source_inline success Was 0 now: 0
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-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Matt Bobrowski Feb. 14, 2024, 7:30 a.m. UTC 
In some situations, if you fail to zero-initialize the
bpf_{prog,map,btf,link}_info structs supplied to the set of LIBBPF
helpers bpf_{prog,map,btf,link}_get_info_by_fd(), you can expect the
helper to return an error. This can possibly leave people in a
situation where they're scratching their heads for an unnnecessary
amount of time. Make an explicit remark about the requirement of
zero-initializing the supplied bpf_{prog,map,btf,link}_info structs
for the respective LIBBPF helpers.

Internally, LIBBPF helpers bpf_{prog,map,btf,link}_get_info_by_fd()
call into bpf_obj_get_info_by_fd() where the bpf(2)
BPF_OBJ_GET_INFO_BY_FD command is used. This specific command is
effectively backed by restrictions enforced by the
bpf_check_uarg_tail_zero() helper. This function ensures that if the
size of the supplied bpf_{prog,map,btf,link}_info structs are larger
than what the kernel can handle, trailing bits are zeroed. This can be
a problem when compiling against UAPI headers that don't necessarily
match the sizes of the same underlying types known to the kernel.

Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
---
 tools/lib/bpf/bpf.h | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Jiri Olsa Feb. 14, 2024, 8:23 a.m. UTC | #1 
On Wed, Feb 14, 2024 at 07:30:25AM +0000, Matt Bobrowski wrote:
> In some situations, if you fail to zero-initialize the
> bpf_{prog,map,btf,link}_info structs supplied to the set of LIBBPF
> helpers bpf_{prog,map,btf,link}_get_info_by_fd(), you can expect the
> helper to return an error. This can possibly leave people in a
> situation where they're scratching their heads for an unnnecessary
> amount of time. Make an explicit remark about the requirement of
> zero-initializing the supplied bpf_{prog,map,btf,link}_info structs
> for the respective LIBBPF helpers.
> 
> Internally, LIBBPF helpers bpf_{prog,map,btf,link}_get_info_by_fd()
> call into bpf_obj_get_info_by_fd() where the bpf(2)
> BPF_OBJ_GET_INFO_BY_FD command is used. This specific command is
> effectively backed by restrictions enforced by the
> bpf_check_uarg_tail_zero() helper. This function ensures that if the
> size of the supplied bpf_{prog,map,btf,link}_info structs are larger
> than what the kernel can handle, trailing bits are zeroed. This can be
> a problem when compiling against UAPI headers that don't necessarily
> match the sizes of the same underlying types known to the kernel.
> 
> Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> ---
>  tools/lib/bpf/bpf.h | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index f866e98b2436..3ed745f99da3 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -500,7 +500,10 @@ LIBBPF_API int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len);
>   * program corresponding to *prog_fd*.
>   *
>   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> - * actual number of bytes written to *info*.
> + * actual number of bytes written to *info*. Note that *info* should be
> + * zero-initialized before calling into this helper. Failing to zero-initialize
> + * *info* under certain circumstances can result in this helper returning an
> + * error.
>   *
>   * @param prog_fd BPF program file descriptor
>   * @param info pointer to **struct bpf_prog_info** that will be populated with
> @@ -517,7 +520,10 @@ LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
>   * map corresponding to *map_fd*.
>   *
>   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> - * actual number of bytes written to *info*.
> + * actual number of bytes written to *info*. Note that *info* should be
> + * zero-initialized before calling into this helper. Failing to zero-initialize
> + * *info* under certain circumstances can result in this helper returning an
> + * error.
>   *
>   * @param map_fd BPF map file descriptor
>   * @param info pointer to **struct bpf_map_info** that will be populated with
> @@ -530,11 +536,14 @@ LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
>  LIBBPF_API int bpf_map_get_info_by_fd(int map_fd, struct bpf_map_info *info, __u32 *info_len);
>  
>  /**
> - * @brief **bpf_btf_get_info_by_fd()** obtains information about the 
> + * @brief **bpf_btf_get_info_by_fd()** obtains information about the
>   * BTF object corresponding to *btf_fd*.
>   *
>   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> - * actual number of bytes written to *info*.
> + * actual number of bytes written to *info*. Note that *info* should be
> + * zero-initialized before calling into this helper. Failing to zero-initialize
> + * *info* under certain circumstances can result in this helper returning an
> + * error.
>   *
>   * @param btf_fd BTF object file descriptor
>   * @param info pointer to **struct bpf_btf_info** that will be populated with
> @@ -551,7 +560,10 @@ LIBBPF_API int bpf_btf_get_info_by_fd(int btf_fd, struct bpf_btf_info *info, __u
>   * link corresponding to *link_fd*.
>   *
>   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> - * actual number of bytes written to *info*.
> + * actual number of bytes written to *info*. Note that *info* should be
> + * zero-initialized before calling into this helper. Failing to zero-initialize
> + * *info* under certain circumstances can result in this helper returning an
> + * error.

this is slightly misleading, because like for uprobe/kprobe multi links we normally
call bpf_link_get_info_by_fd twice, first time with zero initialed info to get the
static data and then again with info filled with user space buffer pointers to get
other data like addresses or cookies.. I think to some extend this is similar also
for bpf_prog_get_info_by_fd

maybe something like:

Note that *info* should be zero-initialized or initialized as expected by the
requested object type. Failing to (zero)initialize *info* under certain circumstances
can result in this helper returning an error.

jirka

>   *
>   * @param link_fd BPF link file descriptor
>   * @param info pointer to **struct bpf_link_info** that will be populated with
> -- 
> 2.43.0.687.g38aa6559b0-goog
> 
> /M
>
Matt Bobrowski Feb. 14, 2024, 8:30 a.m. UTC | #2 
On Wed, Feb 14, 2024 at 09:23:53AM +0100, Jiri Olsa wrote:
> On Wed, Feb 14, 2024 at 07:30:25AM +0000, Matt Bobrowski wrote:
> > In some situations, if you fail to zero-initialize the
> > bpf_{prog,map,btf,link}_info structs supplied to the set of LIBBPF
> > helpers bpf_{prog,map,btf,link}_get_info_by_fd(), you can expect the
> > helper to return an error. This can possibly leave people in a
> > situation where they're scratching their heads for an unnnecessary
> > amount of time. Make an explicit remark about the requirement of
> > zero-initializing the supplied bpf_{prog,map,btf,link}_info structs
> > for the respective LIBBPF helpers.
> > 
> > Internally, LIBBPF helpers bpf_{prog,map,btf,link}_get_info_by_fd()
> > call into bpf_obj_get_info_by_fd() where the bpf(2)
> > BPF_OBJ_GET_INFO_BY_FD command is used. This specific command is
> > effectively backed by restrictions enforced by the
> > bpf_check_uarg_tail_zero() helper. This function ensures that if the
> > size of the supplied bpf_{prog,map,btf,link}_info structs are larger
> > than what the kernel can handle, trailing bits are zeroed. This can be
> > a problem when compiling against UAPI headers that don't necessarily
> > match the sizes of the same underlying types known to the kernel.
> > 
> > Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> > ---
> >  tools/lib/bpf/bpf.h | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index f866e98b2436..3ed745f99da3 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -500,7 +500,10 @@ LIBBPF_API int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len);
> >   * program corresponding to *prog_fd*.
> >   *
> >   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> > - * actual number of bytes written to *info*.
> > + * actual number of bytes written to *info*. Note that *info* should be
> > + * zero-initialized before calling into this helper. Failing to zero-initialize
> > + * *info* under certain circumstances can result in this helper returning an
> > + * error.
> >   *
> >   * @param prog_fd BPF program file descriptor
> >   * @param info pointer to **struct bpf_prog_info** that will be populated with
> > @@ -517,7 +520,10 @@ LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
> >   * map corresponding to *map_fd*.
> >   *
> >   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> > - * actual number of bytes written to *info*.
> > + * actual number of bytes written to *info*. Note that *info* should be
> > + * zero-initialized before calling into this helper. Failing to zero-initialize
> > + * *info* under certain circumstances can result in this helper returning an
> > + * error.
> >   *
> >   * @param map_fd BPF map file descriptor
> >   * @param info pointer to **struct bpf_map_info** that will be populated with
> > @@ -530,11 +536,14 @@ LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
> >  LIBBPF_API int bpf_map_get_info_by_fd(int map_fd, struct bpf_map_info *info, __u32 *info_len);
> >  
> >  /**
> > - * @brief **bpf_btf_get_info_by_fd()** obtains information about the 
> > + * @brief **bpf_btf_get_info_by_fd()** obtains information about the
> >   * BTF object corresponding to *btf_fd*.
> >   *
> >   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> > - * actual number of bytes written to *info*.
> > + * actual number of bytes written to *info*. Note that *info* should be
> > + * zero-initialized before calling into this helper. Failing to zero-initialize
> > + * *info* under certain circumstances can result in this helper returning an
> > + * error.
> >   *
> >   * @param btf_fd BTF object file descriptor
> >   * @param info pointer to **struct bpf_btf_info** that will be populated with
> > @@ -551,7 +560,10 @@ LIBBPF_API int bpf_btf_get_info_by_fd(int btf_fd, struct bpf_btf_info *info, __u
> >   * link corresponding to *link_fd*.
> >   *
> >   * Populates up to *info_len* bytes of *info* and updates *info_len* with the
> > - * actual number of bytes written to *info*.
> > + * actual number of bytes written to *info*. Note that *info* should be
> > + * zero-initialized before calling into this helper. Failing to zero-initialize
> > + * *info* under certain circumstances can result in this helper returning an
> > + * error.
> 
> this is slightly misleading, because like for uprobe/kprobe multi links we normally
> call bpf_link_get_info_by_fd twice, first time with zero initialed info to get the
> static data and then again with info filled with user space buffer pointers to get
> other data like addresses or cookies.. I think to some extend this is similar also
> for bpf_prog_get_info_by_fd

Ah, you're right, it is slightly misleading as the current wording
implies that the supplied buffer must always be zeroed out, when in
reality that's not true.

> maybe something like:
> 
> Note that *info* should be zero-initialized or initialized as expected by the
> requested object type. Failing to (zero)initialize *info* under certain circumstances
> can result in this helper returning an error.

I'll adapt as per your recommendations and then resend this through
once again.

Thanks for the feedback!

/M
diff mbox series

Patch

diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index f866e98b2436..3ed745f99da3 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -500,7 +500,10 @@  LIBBPF_API int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len);
  * program corresponding to *prog_fd*.
  *
  * Populates up to *info_len* bytes of *info* and updates *info_len* with the
- * actual number of bytes written to *info*.
+ * actual number of bytes written to *info*. Note that *info* should be
+ * zero-initialized before calling into this helper. Failing to zero-initialize
+ * *info* under certain circumstances can result in this helper returning an
+ * error.
  *
  * @param prog_fd BPF program file descriptor
  * @param info pointer to **struct bpf_prog_info** that will be populated with
@@ -517,7 +520,10 @@  LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
  * map corresponding to *map_fd*.
  *
  * Populates up to *info_len* bytes of *info* and updates *info_len* with the
- * actual number of bytes written to *info*.
+ * actual number of bytes written to *info*. Note that *info* should be
+ * zero-initialized before calling into this helper. Failing to zero-initialize
+ * *info* under certain circumstances can result in this helper returning an
+ * error.
  *
  * @param map_fd BPF map file descriptor
  * @param info pointer to **struct bpf_map_info** that will be populated with
@@ -530,11 +536,14 @@  LIBBPF_API int bpf_prog_get_info_by_fd(int prog_fd, struct bpf_prog_info *info,
 LIBBPF_API int bpf_map_get_info_by_fd(int map_fd, struct bpf_map_info *info, __u32 *info_len);
 
 /**
- * @brief **bpf_btf_get_info_by_fd()** obtains information about the 
+ * @brief **bpf_btf_get_info_by_fd()** obtains information about the
  * BTF object corresponding to *btf_fd*.
  *
  * Populates up to *info_len* bytes of *info* and updates *info_len* with the
- * actual number of bytes written to *info*.
+ * actual number of bytes written to *info*. Note that *info* should be
+ * zero-initialized before calling into this helper. Failing to zero-initialize
+ * *info* under certain circumstances can result in this helper returning an
+ * error.
  *
  * @param btf_fd BTF object file descriptor
  * @param info pointer to **struct bpf_btf_info** that will be populated with
@@ -551,7 +560,10 @@  LIBBPF_API int bpf_btf_get_info_by_fd(int btf_fd, struct bpf_btf_info *info, __u
  * link corresponding to *link_fd*.
  *
  * Populates up to *info_len* bytes of *info* and updates *info_len* with the
- * actual number of bytes written to *info*.
+ * actual number of bytes written to *info*. Note that *info* should be
+ * zero-initialized before calling into this helper. Failing to zero-initialize
+ * *info* under certain circumstances can result in this helper returning an
+ * error.
  *
  * @param link_fd BPF link file descriptor
  * @param info pointer to **struct bpf_link_info** that will be populated with