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

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

[Jesper Dangaard Brouer]

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.

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,

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

[Andrii Nakryiko]

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

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

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

[Jesper Dangaard Brouer]

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

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

[Andrii Nakryiko]

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
>