Re: [PATCH bpf-next] bpf: Fix map_check_no_btf return code

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, May 26, 2020 at 11:35 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Tue, 26 May 2020 11:16:50 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Tue, May 26, 2020 at 7:59 AM Jesper Dangaard Brouer
> > <brouer@redhat.com> wrote:
> > >
> > > When a BPF-map type doesn't support having a BTF info associated, the
> > > bpf_map_ops->map_check_btf is set to map_check_no_btf(). This function
> > > map_check_no_btf() currently returns -ENOTSUPP, which result in a very
> > > confusing error message in libbpf, see below.
> > >
> > > The errno ENOTSUPP is part of the kernels internal errno in file
> > > include/linux/errno.h. As is stated in the file, these "should never be seen
> > > by user programs."
> > >
> > > Choosing errno EUCLEAN instead, which translated to "Structure needs
> > > cleaning" by strerror(3). This hopefully leads people to think about data
> > > structures which BTF is all about.
> >
> > How about instead of tweaking error code
>
> Regardless we still need to change the error code used, as strerror(3)
> cannot convert it to something meaningful.  As the code comment says
> this errno "should never be seen by user programs.".

"Structure needs cleaning" doesn't really make sense to me either, to
be honest. If -524 bothers you so much, maybe switch to EOPNOTSUPP
(with alias ENOTSUP in user-space)? Or we can just handle this case
better in libbpf for better user error? Either way there will be a lot
of old kernels around that will still produce such error.

>
> My notes are here:
>  https://github.com/xdp-project/xdp-project/blob/BTF01-notes.public/areas/core/BTF_01_notes.org
>
> > we actually just add support
> > for BTF key/values for all maps. For special maps, we can just enforce
> > that BTF is 4-byte integer (or typedef of that), so that in practice
> > you'll be defining it as:
> >
> > struct {
> >     __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
> >     __type(key, u32);
> >     __type(value, u32);
> > } my_map SEC(".maps");
> >
> > and it will just work?
>
> Nope, this will also fail.

Why? If kernel supports 4-byte integers as BTF types for key/value for
such maps, then what would fail in this case?

>
> I'm all for adding support for more BPF-maps in follow up patches.  I
> will soon be adding support for cpumap and devmap.  And I will likely
> be asking all kind of weird questions, I hope I can get some help from
> you to figure out all the details ;-)

Of course.

>
> > >
> > > Before this change end-users of libbpf will see:
> > >  libbpf: Error in bpf_create_map_xattr(cpu_map):ERROR: strerror_r(-524)=22(-524). Retrying without BTF.
> > >
> > > After this change end-users of libbpf will see:
> > >  libbpf: Error in bpf_create_map_xattr(cpu_map):Structure needs cleaning(-117). Retrying without BTF.
> > >
> > > Fixes: e8d2bec04579 ("bpf: decouple btf from seq bpf fs dump and enable more maps")
> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > > ---
> > >  kernel/bpf/syscall.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index d13b804ff045..ecde7d938421 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -732,7 +732,7 @@ int map_check_no_btf(const struct bpf_map *map,
> > >                      const struct btf_type *key_type,
> > >                      const struct btf_type *value_type)
> > >  {
> > > -       return -ENOTSUPP;
> > > +       return -EUCLEAN;
> > >  }
> > >
> > >  static int map_check_btf(struct bpf_map *map, const struct btf *btf,
> > >
>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>