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@xxxxxxxxxxxxx> 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@xxxxxxxxxx>    
> > > > >
> > > > > 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.