[PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Toke Høiland-Jørgensen]

When the kptr_restrict sysctl is set, the kernel can fail to return
jited_ksyms or jited_prog_insns, but still have positive values in
nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
to dump the program because it only checks the len fields not the actual
pointers to the instructions and ksyms.

Fix this by adding the missing checks.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
v2:
  - The sysctl causing this is kptr_restrict, not bpf_jit_harden; update commit
    msg to get this right (Martin).

 tools/bpf/bpftool/prog.c          | 2 +-
 tools/bpf/bpftool/xlated_dumper.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 4535c863d2cd..2ce9c5ba1934 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -493,7 +493,7 @@ static int do_dump(int argc, char **argv)
 
 	info = &info_linear->info;
 	if (mode == DUMP_JITED) {
-		if (info->jited_prog_len == 0) {
+		if (info->jited_prog_len == 0 || !info->jited_prog_insns) {
 			p_info("no instructions returned");
 			goto err_free;
 		}
diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
index 494d7ae3614d..5b91ee65a080 100644
--- a/tools/bpf/bpftool/xlated_dumper.c
+++ b/tools/bpf/bpftool/xlated_dumper.c
@@ -174,7 +174,7 @@ static const char *print_call(void *private_data,
 	struct kernel_sym *sym;
 
 	if (insn->src_reg == BPF_PSEUDO_CALL &&
-	    (__u32) insn->imm < dd->nr_jited_ksyms)
+	    (__u32) insn->imm < dd->nr_jited_ksyms && dd->jited_ksyms)
 		address = dd->jited_ksyms[insn->imm];
 
 	sym = kernel_syms_search(dd, address);
-- 
2.24.0

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Martin Lau]

On Tue, Dec 10, 2019 at 07:14:12PM +0100, Toke Høiland-Jørgensen wrote:
> When the kptr_restrict sysctl is set, the kernel can fail to return
> jited_ksyms or jited_prog_insns, but still have positive values in
> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> to dump the program because it only checks the len fields not the actual
> pointers to the instructions and ksyms.
> 
> Fix this by adding the missing checks.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Jakub Kicinski]

On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:
> When the kptr_restrict sysctl is set, the kernel can fail to return
> jited_ksyms or jited_prog_insns, but still have positive values in
> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> to dump the program because it only checks the len fields not the actual
> pointers to the instructions and ksyms.
> 
> Fix this by adding the missing checks.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")

and

Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")

?

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Toke Høiland-Jørgensen]

Jakub Kicinski <jakub.kicinski@netronome.com> writes:

> On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:
>> When the kptr_restrict sysctl is set, the kernel can fail to return
>> jited_ksyms or jited_prog_insns, but still have positive values in
>> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
>> to dump the program because it only checks the len fields not the actual
>> pointers to the instructions and ksyms.
>> 
>> Fix this by adding the missing checks.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
>
> and
>
> Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")
>
> ?

Yeah, guess so? Although I must admit it's not quite clear to me whether
bpftool gets stable backports, or if it follows the "only moving
forward" credo of libbpf?

Anyhow, I don't suppose it'll hurt to have the Fixes: tag(s) in there;
does Patchwork pick these up (or can you guys do that when you apply
this?), or should I resend?

-Toke

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Jakub Kicinski]

On Tue, 10 Dec 2019 22:09:55 +0100, Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> > On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:  
> >> When the kptr_restrict sysctl is set, the kernel can fail to return
> >> jited_ksyms or jited_prog_insns, but still have positive values in
> >> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> >> to dump the program because it only checks the len fields not the actual
> >> pointers to the instructions and ksyms.
> >> 
> >> Fix this by adding the missing checks.
> >> 
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>  
> >
> > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> >
> > and
> >
> > Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")
> >
> > ?  
> 
> Yeah, guess so? Although I must admit it's not quite clear to me whether
> bpftool gets stable backports, or if it follows the "only moving
> forward" credo of libbpf?

bpftool does not have a GH repo, and seeing strength of Alexei's
arguments in the recent discussion - I don't think it will. So no
reason for bpftool to be "special" ❄️.

Then again seeing Andrii's zeal for pushing the codegen stuff into
bpftool, maybe Facebook's intention is to make it so.

Hard to tell what to do when standard practices don't apply, sigh.

> Anyhow, I don't suppose it'll hurt to have the Fixes: tag(s) in there;
> does Patchwork pick these up (or can you guys do that when you apply
> this?), or should I resend?

I don't think it does, but perhaps Daniel's scripts do.

Either way I don't think it's worth a resend.

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Alexei Starovoitov]

On Tue, Dec 10, 2019 at 01:24:28PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 22:09:55 +0100, Toke Høiland-Jørgensen wrote:
> > Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> > > On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:  
> > >> When the kptr_restrict sysctl is set, the kernel can fail to return
> > >> jited_ksyms or jited_prog_insns, but still have positive values in
> > >> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> > >> to dump the program because it only checks the len fields not the actual
> > >> pointers to the instructions and ksyms.
> > >> 
> > >> Fix this by adding the missing checks.
> > >> 
> > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>  
> > >
> > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> > >
> > > and
> > >
> > > Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")
> > >
> > > ?  
> > 
> > Yeah, guess so? Although I must admit it's not quite clear to me whether
> > bpftool gets stable backports, or if it follows the "only moving
> > forward" credo of libbpf?
> 
> bpftool does not have a GH repo, and seeing strength of Alexei's
> arguments in the recent discussion - I don't think it will. So no
> reason for bpftool to be "special"

bpftool always was and will be a special user of libbpf.

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Jakub Kicinski]

On Tue, 10 Dec 2019 13:31:50 -0800, Alexei Starovoitov wrote:
> On Tue, Dec 10, 2019 at 01:24:28PM -0800, Jakub Kicinski wrote:
> > On Tue, 10 Dec 2019 22:09:55 +0100, Toke Høiland-Jørgensen wrote:  
> > > Jakub Kicinski <jakub.kicinski@netronome.com> writes:  
> > > > On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:    
> > > >> When the kptr_restrict sysctl is set, the kernel can fail to return
> > > >> jited_ksyms or jited_prog_insns, but still have positive values in
> > > >> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> > > >> to dump the program because it only checks the len fields not the actual
> > > >> pointers to the instructions and ksyms.
> > > >> 
> > > >> Fix this by adding the missing checks.
> > > >> 
> > > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>    
> > > >
> > > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> > > >
> > > > and
> > > >
> > > > Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")
> > > >
> > > > ?    
> > > 
> > > Yeah, guess so? Although I must admit it's not quite clear to me whether
> > > bpftool gets stable backports, or if it follows the "only moving
> > > forward" credo of libbpf?  
> > 
> > bpftool does not have a GH repo, and seeing strength of Alexei's
> > arguments in the recent discussion - I don't think it will. So no
> > reason for bpftool to be "special"  
> 
> bpftool always was and will be a special user of libbpf.

There we go again. Making proclamations without any justification or
explanation.

Maybe there is a language barrier between us, but I wrote the initial
bpftool code, so I don't see how you (who authored one patch) can say
what it was or is. Do you mean to say what you intend to make it?

bpftool was intended to be a CLI to BPF _kernel_ interface. libbpf was
just the library that we all agreed to use moving forward for ELF
loading.

I'm not going to argue with you again. You kept bad mouthing iproute2
and then your only argument was the reviews sometimes take longer than
24 hours. Which I'm sure you have a lot of experience with:

  iproute2$ git log --author=Starov --oneline 
4bfe68253670 iptnl: add support for collect_md flag in IPv4 and IPv6 tunnels

  iproute2$ git log --author=fb.com --oneline 
3da6d055d93f bpf: add btf func and func_proto kind support
7a04dd84a7f9 bpf: check map symbol type properly with newer llvm compiler
73451259daaa tc: fix ipv6 filter selector attribute for some prefix lengths
0b4ea60b5a48 bpf: Add support for IFLA_XDP_PROG_ID
4bfe68253670 iptnl: add support for collect_md flag in IPv4 and IPv6 tunnels
414aeec90f82 ss: Add tcp_info fields data_segs_in/out
409998c5a4eb iproute: ip-gue/ip-fou manpages

Upstreaming bpftool was a big mistake, but we live and we learn.

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Alexei Starovoitov]

On Tue, Dec 10, 2019 at 01:52:05PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 13:31:50 -0800, Alexei Starovoitov wrote:
> > On Tue, Dec 10, 2019 at 01:24:28PM -0800, Jakub Kicinski wrote:
> > > On Tue, 10 Dec 2019 22:09:55 +0100, Toke Høiland-Jørgensen wrote:  
> > > > Jakub Kicinski <jakub.kicinski@netronome.com> writes:  
> > > > > On Tue, 10 Dec 2019 19:14:12 +0100, Toke Høiland-Jørgensen wrote:    
> > > > >> When the kptr_restrict sysctl is set, the kernel can fail to return
> > > > >> jited_ksyms or jited_prog_insns, but still have positive values in
> > > > >> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> > > > >> to dump the program because it only checks the len fields not the actual
> > > > >> pointers to the instructions and ksyms.
> > > > >> 
> > > > >> Fix this by adding the missing checks.
> > > > >> 
> > > > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>    
> > > > >
> > > > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> > > > >
> > > > > and
> > > > >
> > > > > Fixes: f84192ee00b7 ("tools: bpftool: resolve calls without using imm field")
> > > > >
> > > > > ?    
> > > > 
> > > > Yeah, guess so? Although I must admit it's not quite clear to me whether
> > > > bpftool gets stable backports, or if it follows the "only moving
> > > > forward" credo of libbpf?  
> > > 
> > > bpftool does not have a GH repo, and seeing strength of Alexei's
> > > arguments in the recent discussion - I don't think it will. So no
> > > reason for bpftool to be "special"  
> > 
> > bpftool always was and will be a special user of libbpf.
> 
> There we go again. Making proclamations without any justification or
> explanation.
> 
> Maybe there is a language barrier between us, but I wrote the initial
> bpftool code, so I don't see how you (who authored one patch) can say
> what it was or is. Do you mean to say what you intend to make it?

When code lands it becomes the part of the code that maintainers keep in the
best shape possible considering contributions from many parties. Original
author of the code is equal to everyone else who submits patches. The job of
the maintainer is to mediate folks who contribute the patches. Sounds like you
expected to own bpftool as a single owner. I don't think kernel is such place.
In most projects I'm aware of the OWNERS file is discouraged. The kernel is
such project. The kernel has MAINTAINERS file which lists people most
knowledgeable in the area and who's job is to keep the code in good shape,
consider long term growth, etc. Maintainers are not owners of the code.

bpftool became way more than what it was when initially landed. Just like
libbpf was implemented mainly by huawei folks and used in perf only. Now perf
is a minority and original authors are not contributing any more. I'm sure you
thought of bpftool as cli for sys_bpf only. Well thankfully it outgrew this
narrow view point. bpftool btf dump file doesn't call sys_bpf at all.
bpftool perf | net using several other kernel apis. I think it's a good growth
trajectory for bpftool. More people use it and like it.

> Upstreaming bpftool was a big mistake, but we live and we learn

ok. Not going count on your help anymore.
Since you're not interesting in helping please don't stall it either.

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Daniel Borkmann]

On Tue, Dec 10, 2019 at 10:09:55PM +0100, Toke Høiland-Jørgensen wrote:
[...]
> Anyhow, I don't suppose it'll hurt to have the Fixes: tag(s) in there;
> does Patchwork pick these up (or can you guys do that when you apply
> this?), or should I resend?

Fixes tags should /always/ be present if possible, since they help to provide
more context even if the buggy commit was in bpf-next, for example.

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.net> writes:

> On Tue, Dec 10, 2019 at 10:09:55PM +0100, Toke Høiland-Jørgensen wrote:
> [...]
>> Anyhow, I don't suppose it'll hurt to have the Fixes: tag(s) in there;
>> does Patchwork pick these up (or can you guys do that when you apply
>> this?), or should I resend?
>
> Fixes tags should /always/ be present if possible, since they help to provide
> more context even if the buggy commit was in bpf-next, for example.

ACK, will do. Thank you for picking them up for this patch (did you do
that manually, or is this part of your scripts?)

-Toke

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Daniel Borkmann]

On Wed, Dec 11, 2019 at 02:20:11PM +0100, Toke Høiland-Jørgensen wrote:
> Daniel Borkmann <daniel@iogearbox.net> writes:
> > On Tue, Dec 10, 2019 at 10:09:55PM +0100, Toke Høiland-Jørgensen wrote:
> > [...]
> >> Anyhow, I don't suppose it'll hurt to have the Fixes: tag(s) in there;
> >> does Patchwork pick these up (or can you guys do that when you apply
> >> this?), or should I resend?
> >
> > Fixes tags should /always/ be present if possible, since they help to provide
> > more context even if the buggy commit was in bpf-next, for example.
> 
> ACK, will do. Thank you for picking them up for this patch (did you do
> that manually, or is this part of your scripts?)

Manually right now, but if you have some cycles, happy to pick up a patch [0]. :)

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/dborkman/pw.git/

Re: [PATCH bpf v2] bpftool: Don't crash on missing jited insns or ksyms

[Daniel Borkmann]

On Tue, Dec 10, 2019 at 07:14:12PM +0100, Toke Høiland-Jørgensen wrote:
> When the kptr_restrict sysctl is set, the kernel can fail to return
> jited_ksyms or jited_prog_insns, but still have positive values in
> nr_jited_ksyms and jited_prog_len. This causes bpftool to crash when trying
> to dump the program because it only checks the len fields not the actual
> pointers to the instructions and ksyms.
> 
> Fix this by adding the missing checks.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Applied, thanks!