bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
@ 2020-05-27 11:33 Jesper Dangaard Brouer
  2020-05-27 22:59 ` Andrii Nakryiko
  0 siblings, 1 reply; 6+ messages in thread
From: Jesper Dangaard Brouer @ 2020-05-27 11:33 UTC (permalink / raw)
  To: bpf
  Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
	Jakub Kicinski, Andrii Nakryiko

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". This is not a not a standard Unix error.

This should likely have been EOPNOTSUPP instead. This seems to be a common
mistake, that even checkpatch tries to catch see commit 6b9ea5ff5abd
("checkpatch: warn about uses of ENOTSUPP").

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):Operation not supported(-95). Retrying without BTF.

V2: Use EOPNOTSUPP instead of EUCLEAN.

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..e4e0a0c5192c 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 -EOPNOTSUPP;
 }
 
 static int map_check_btf(struct bpf_map *map, const struct btf *btf,



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
  2020-05-27 11:33 [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code Jesper Dangaard Brouer
@ 2020-05-27 22:59 ` Andrii Nakryiko
  2020-05-28  7:08   ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-05-27 22:59 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Jakub Kicinski

On Wed, May 27, 2020 at 4:34 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". This is not a not a standard Unix error.
>
> This should likely have been EOPNOTSUPP instead. This seems to be a common
> mistake, that even checkpatch tries to catch see commit 6b9ea5ff5abd
> ("checkpatch: warn about uses of ENOTSUPP").
>
> 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):Operation not supported(-95). Retrying without BTF.
>
> V2: Use EOPNOTSUPP instead of EUCLEAN.
>
> Fixes: e8d2bec04579 ("bpf: decouple btf from seq bpf fs dump and enable more maps")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---

I don't mind this change, per se, mostly because I think it doesn't matter. But:

$ rg ENOTSUPP kernel/bpf | wc -l
42
$ rg EOPNOTSUPP kernel/bpf | wc -l
8

So 5 to 1 in favor of ENOTSUPP in purely BPF sources.

Globally across kernel sources the picture is different, though:

$ rg ENOTSUPP | wc -l
1597
$ rg EOPNOTSUPP | wc -l
6982

I didn't audit if those errors can get eventually propagated to
user-space, but I'd imagine that most would. So, despite what that
errno.h header says, EOPNOTSUPP is quite widely used still.

But regardless, can you please reply on v1 thread why adding support
for BTF to these special maps (that do not support BTF right now)
won't be a better solution and won't work (as you claimed)?


>  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..e4e0a0c5192c 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 -EOPNOTSUPP;
>  }
>
>  static int map_check_btf(struct bpf_map *map, const struct btf *btf,
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
  2020-05-27 22:59 ` Andrii Nakryiko
@ 2020-05-28  7:08   ` Jesper Dangaard Brouer
  2020-05-28 18:21     ` Andrii Nakryiko
  0 siblings, 1 reply; 6+ messages in thread
From: Jesper Dangaard Brouer @ 2020-05-28  7:08 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Jakub Kicinski, brouer

On Wed, 27 May 2020 15:59:46 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> But regardless, can you please reply on v1 thread why adding support
> for BTF to these special maps (that do not support BTF right now)
> won't be a better solution and won't work (as you claimed)?

(I will reply here instead of on v1) and I have not claimed it won't
work.  It will work, but we loose an opportunity if we just allow BTF
across the board, without using it for per map validation.

We have an opportunity with these special maps, that do not support BTF
right now.  The value field in these maps are actually a UAPI and also
kABI (Binary).  

Simply describing the struct with BTF is nice, but only helpful to make
the end-user understand they binary layout.  On the kernel side we are
still stuck with a kABI that can only be end-extended and size increased.
I want to use BTF on the kernel-side to validate and enforce that user
provided the expected "named" layout, and possibly reject it.  This
gives us a layer that can provide a flexible kABI.  From my current
understanding of the kernel side code that parse/walk BTF, I we can
actually have flexible offsets (for e.g. structs) in the binary value,
and remap those on the kernel side.  Enforcing a named layout when we
enable BTF for these maps, will give us binary flexibility for future
changes.  I hope you agree?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
  2020-05-28  7:08   ` Jesper Dangaard Brouer
@ 2020-05-28 18:21     ` Andrii Nakryiko
  2020-05-28 18:38       ` Alexei Starovoitov
  0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2020-05-28 18:21 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Jakub Kicinski

On Thu, May 28, 2020 at 12:08 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Wed, 27 May 2020 15:59:46 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > But regardless, can you please reply on v1 thread why adding support
> > for BTF to these special maps (that do not support BTF right now)
> > won't be a better solution and won't work (as you claimed)?
>
> (I will reply here instead of on v1) and I have not claimed it won't

Well, maybe I misinterpreted your response from that thread, but not
sure how I should have interpreted it differently:

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


> work.  It will work, but we loose an opportunity if we just allow BTF
> across the board, without using it for per map validation.

I don't see how we are losing any extensibility, honestly. Right now
we can allow simple "4-byte integer". If we need to extend it in the
future, we'll allow 4-byte integer (for backwards compatibility), and
whatever struct makes sense for extended use case. But maybe I don't
completely understand what you are after here. See below.

>
> We have an opportunity with these special maps, that do not support BTF
> right now.  The value field in these maps are actually a UAPI and also
> kABI (Binary).
>
> Simply describing the struct with BTF is nice, but only helpful to make
> the end-user understand they binary layout.  On the kernel side we are
> still stuck with a kABI that can only be end-extended and size increased.
> I want to use BTF on the kernel-side to validate and enforce that user
> provided the expected "named" layout, and possibly reject it.  This
> gives us a layer that can provide a flexible kABI.  From my current
> understanding of the kernel side code that parse/walk BTF, I we can
> actually have flexible offsets (for e.g. structs) in the binary value,
> and remap those on the kernel side.  Enforcing a named layout when we
> enable BTF for these maps, will give us binary flexibility for future
> changes.  I hope you agree?

Can you expand on this named approach and what are the use cases you
are seeing? I guess it's something like what is done for
bpf_spin_lock, where we require struct with well-defined name and
field name, etc. But please elaborate here.


My biggest grudge with changing error code is that old error code will
still be used in older kernels, so if libbpf were to help users with
more helpful message, it now needs to support both error codes,
forever, potentially depending on kernel version. This constant
splitting of logic is annoying, so I'd rather avoid it.

>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
  2020-05-28 18:21     ` Andrii Nakryiko
@ 2020-05-28 18:38       ` Alexei Starovoitov
  2020-05-28 19:41         ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2020-05-28 18:38 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, bpf, Daniel Borkmann, Jakub Kicinski

On Thu, May 28, 2020 at 11:21 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
>
> My biggest grudge with changing error code is that old error code will
> still be used in older kernels, so if libbpf were to help users with
> more helpful message, it now needs to support both error codes,
> forever, potentially depending on kernel version. This constant
> splitting of logic is annoying, so I'd rather avoid it.

+1.

I think what this patch is trying to do is to fix strerror() lack of
understanding of error code on the kernel side by changing
the error code.
There are plenty of similar places in the kernel.
I think it's better to fix strerror via wrapper that understand kernel
error codes.
There probably will be more than one such ENOTSUPP error.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code
  2020-05-28 18:38       ` Alexei Starovoitov
@ 2020-05-28 19:41         ` Daniel Borkmann
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2020-05-28 19:41 UTC (permalink / raw)
  To: Alexei Starovoitov, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, bpf, Daniel Borkmann, Jakub Kicinski

On 5/28/20 8:38 PM, Alexei Starovoitov wrote:
> On Thu, May 28, 2020 at 11:21 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> My biggest grudge with changing error code is that old error code will
>> still be used in older kernels, so if libbpf were to help users with
>> more helpful message, it now needs to support both error codes,
>> forever, potentially depending on kernel version. This constant
>> splitting of logic is annoying, so I'd rather avoid it.
> 
> +1.
> 
> I think what this patch is trying to do is to fix strerror() lack of
> understanding of error code on the kernel side by changing
> the error code.
> There are plenty of similar places in the kernel.
> I think it's better to fix strerror via wrapper that understand kernel
> error codes.
> There probably will be more than one such ENOTSUPP error.

Fully agree, libbpf is the better place to provide a more user friendly
error especially given the older kernel case.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-05-28 19:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 11:33 [PATCH bpf-next V2] bpf: Fix map_check_no_btf return code Jesper Dangaard Brouer
2020-05-27 22:59 ` Andrii Nakryiko
2020-05-28  7:08   ` Jesper Dangaard Brouer
2020-05-28 18:21     ` Andrii Nakryiko
2020-05-28 18:38       ` Alexei Starovoitov
2020-05-28 19:41         ` Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).