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

[Matt Bobrowski <mattbobrowski@google.com>]

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