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

[Jesper Dangaard Brouer <brouer@redhat.com>]

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.".

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.

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 ;-)

> >
> > 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