On Tue, Nov 20, 2018 at 01:19:05PM -0800, Stanislav Fomichev wrote: > On 11/20, Alexei Starovoitov wrote: > > On Mon, Nov 19, 2018 at 04:46:25PM -0800, Stanislav Fomichev wrote: > > > [Recent commit 23499442c319 ("bpf: libbpf: retry map creation without > > > the name") fixed this issue for maps, let's do the same for programs.] > > > > > > Since commit 88cda1c9da02 ("bpf: libbpf: Provide basic API support > > > to specify BPF obj name"), libbpf unconditionally sets bpf_attr->name > > > for programs. Pre v4.14 kernels don't know about programs names and > > > return an error about unexpected non-zero data. Retry sys_bpf without > > > a program name to cover older kernels. > > > > > > Signed-off-by: Stanislav Fomichev <s...@google.com> > > > --- > > > tools/lib/bpf/bpf.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > > index 961e1b9fc592..cbe9d757c646 100644 > > > --- a/tools/lib/bpf/bpf.c > > > +++ b/tools/lib/bpf/bpf.c > > > @@ -212,6 +212,16 @@ int bpf_load_program_xattr(const struct > > > bpf_load_program_attr *load_attr, > > > if (fd >= 0 || !log_buf || !log_buf_sz) > > > return fd; > > > > > > + if (fd < 0 && errno == E2BIG && load_attr->name) { > > > + /* Retry the same syscall, but without the name. > > > + * Pre v4.14 kernels don't support prog names. > > > + */ > > > > I'm afraid that will put unnecessary stress on the kernel. > > This check needs to be tighter. > > Like E2BIG and anything in the log_buf probably means that > > E2BIG came from the verifier and nothing to do with prog_name. > > Asking kernel to repeat is an unnecessary work. > > > > In general we need to think beyond this single prog_name field. > > There are bunch of other fields in bpf_load_program_xattr() and older > > kernels > > won't support them. Are we going to zero them out one by one > > and retry? I don't think that would be practical. > I general, we don't want to zero anything out. However, > for this particular problem the rationale is the following: > In commit 88cda1c9da02 we started unconditionally setting {prog,map}->name > from the 'higher' libbpfc layer which breaks users on the older kernels. > > > Also libbpf silently ignoring prog_name is not great for debugging. > > A warning is needed. > > But it cannot be done out of lib/bpf/bpf.c, since it's a set of syscall > > wrappers. > > Imo such "old kernel -> lets retry" feature should probably be done > > at lib/bpf/libbpf.c level. inside load_program(). > For maps bpftools calls bpf_create_map_xattr directly, that's why > for maps I did the retry on the lower level (and why for programs I initially > thought about doing the same). However, in this case maybe asking > user to omit 'name' argument might be a better option. > > For program names, I agree, we might think about doing it on the higher > level (although I'm not sure whether we want to have different API > expectations, i.e. bpf_create_map_xattr ignoring the name and > bpf_load_program_xattr not ignoring the name). > > So given that rationale above, what do you think is the best way to > move forward? > 1. Same patch, but tighten the retry check inside bpf_load_program_xattr ? > 2. Move this retry logic into load_program and have different handling > for bpf_create_map_xattr vs bpf_load_program_xattr ? > 3. Do 2 and move the retry check for maps from bpf_create_map_xattr > into bpf_object__create_maps ? > > (I'm slightly leaning towards #3)
me too. I think it's cleaner for maps to do it in bpf_object__create_maps(). Originally bpf.c was envisioned to be a thin layer on top of bpf syscall. Whereas 'smart bits' would go into libbpf.c Right now this boundary is unfortunately blurry. May be as #4 long term option we'll introduce another 'smart' layer between bpf.c that will assume the latest kernel and libbpf.c that deals with elf. May be will call this new layer a 'compat' layer? For now I think doing #3 as you suggested is probably the best short term.