From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stanislav Fomichev Subject: Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin Date: Mon, 12 Nov 2018 14:10:11 -0800 Message-ID: <20181112221011.qqqkexvr3mw5clnr@mini-arch> References: <20181109162146.78019-1-sdf@google.com> <20181109162146.78019-3-sdf@google.com> <20181112215337.5ztvb5v7cf4kzr2h@kafai-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Stanislav Fomichev , "netdev@vger.kernel.org" , "linux-kselftest@vger.kernel.org" , "ast@kernel.org" , "daniel@iogearbox.net" , "shuah@kernel.org" , "jakub.kicinski@netronome.com" , "quentin.monnet@netronome.com" , Roman Gushchin , "jiong.wang@netronome.com" , "bhole_prashant_q7@lab.ntt.co.jp" , "john.fastabend@gmail.com" , "jbenc@redhat.com" , "treeze.taeung@gmail.com" , Yonghong Song , Okash Khawaja , "sandipan@linux To: Martin Lau Return-path: Received: from mail-pf1-f195.google.com ([209.85.210.195]:41138 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729271AbeKMIFU (ORCPT ); Tue, 13 Nov 2018 03:05:20 -0500 Received: by mail-pf1-f195.google.com with SMTP id e22-v6so4951232pfn.8 for ; Mon, 12 Nov 2018 14:10:13 -0800 (PST) Content-Disposition: inline In-Reply-To: <20181112215337.5ztvb5v7cf4kzr2h@kafai-mbp.dhcp.thefacebook.com> Sender: netdev-owner@vger.kernel.org List-ID: On 11/12, Martin Lau wrote: > On Fri, Nov 09, 2018 at 08:21:41AM -0800, Stanislav Fomichev wrote: > [ ... ] > > @@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj) > > } > > > > static struct bpf_program * > > -__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) > > +__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i) > > { > > - size_t idx; > > + ssize_t idx; > > > > if (!obj->programs) > > return NULL; > > - /* First handler */ > > - if (prev == NULL) > > - return &obj->programs[0]; > > > > - if (prev->obj != obj) { > > + if (p->obj != obj) { > > pr_warning("error: program handler doesn't match object\n"); > > return NULL; > > } > > > > - idx = (prev - obj->programs) + 1; > > - if (idx >= obj->nr_programs) > > + idx = (p - obj->programs) + i; > > + if (idx >= obj->nr_programs || idx < 0) > > return NULL; > > return &obj->programs[idx]; > > } > > @@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) > > { > > struct bpf_program *prog = prev; > > > > + if (prev == NULL) > > + return obj->programs; > > + > This patch breaks the behavior introduced in > commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"): > "Make bpf_program__next() skip over '.text' section if object file > has pseudo calls. The '.text' section is hardly a program in that > case, it's more of a storage for code of functions other than main." > > For example, the userspace could have been doing: > prog = bpf_program__next(NULL, obj); > bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT); > bpf_object__load(obj); > > For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, > the prog returned by bpf_program__next() could be in ".text" instead of > the main bpf program. The next bpf_program__set_type() has > no effect to the main program. The following bpf_object__load() > will catch user in surprise with the main bpf prog in > the wrong BPF_PROG_TYPE. Will something like the following fix your concern? (plus, assuming the same for prev): --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -2216,8 +2216,11 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) { struct bpf_program *prog = prev; - if (prev == NULL) - return obj->programs; + if (prev == NULL) { + prog = obj->programs; + if (!prog || !bpf_program__is_function_storage(prog, obj)) + return prog; + } do { prog = __bpf_program__iter(prog, obj, 1); Any suggestions for a better way to do it? > > do { > > - prog = __bpf_program__next(prog, obj); > > + prog = __bpf_program__iter(prog, obj, 1); > > + } while (prog && bpf_program__is_function_storage(prog, obj)); > > + > > + return prog; > > +} > > + > > +struct bpf_program * > > +bpf_program__prev(struct bpf_program *next, struct bpf_object *obj) > > +{ > > + struct bpf_program *prog = next; > > + > > + if (next == NULL) { > > + if (!obj->nr_programs) > > + return NULL; > > + return obj->programs + obj->nr_programs - 1; > > + } > > + > > + do { > > + prog = __bpf_program__iter(prog, obj, -1); > > } while (prog && bpf_program__is_function_storage(prog, obj)); > > > > return prog;