* [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 related [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).