Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

[Stanislav Fomichev]

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;