[v5,bpf-next,2/7] libbpf: cleanup after partial failure in bpf_object__pin
diff mbox series

Message ID 20181109162146.78019-3-sdf@google.com
State Accepted
Commit 0c19a9fbc9cdba29c7effb34fd5a97226bf934e6
Headers show
Series
  • bpftool: support loading flow dissector
Related show

Commit Message

Stanislav Fomichev Nov. 9, 2018, 4:21 p.m. UTC
bpftool will use bpf_object__pin in the next commits to pin all programs
and maps from the file; in case of a partial failure, we need to get
back to the clean state (undo previous program/map pins).

As part of a cleanup, I've added and exported separate routines to
pin all maps (bpf_object__pin_maps) and progs (bpf_object__pin_programs)
of an object.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/lib/bpf/libbpf.c | 324 ++++++++++++++++++++++++++++++++++++++---
 tools/lib/bpf/libbpf.h |  18 +++
 2 files changed, 319 insertions(+), 23 deletions(-)

Comments

Martin KaFai Lau Nov. 12, 2018, 9:53 p.m. UTC | #1
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.


>  	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;
Stanislav Fomichev Nov. 12, 2018, 10:10 p.m. UTC | #2
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;
Martin KaFai Lau Nov. 12, 2018, 11:10 p.m. UTC | #3
On Mon, Nov 12, 2018 at 02:10:11PM -0800, Stanislav Fomichev wrote:
> 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?
I think that would work.  The bpf_program__prev() will need the same
treatment though...

Here is my mostly untested fix to unblock my other dev works.  It moves
the very first NULL check back to __bpf_program__iter():

From de1c89ae1768e756825a6874268b5b1686695c93 Mon Sep 17 00:00:00 2001
From: Martin KaFai Lau <kafai@fb.com>
Date: Mon, 12 Nov 2018 14:52:39 -0800
Subject: [PATCH] bpf: libbpf: Fix bpf_program__next() API

This patch restores the behavior in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
such that bpf_program__next() does not return pseudo programs in ".text".

Fixes: 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e827542ffa3a..a01eb9584e52 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
 {
+	size_t nr_programs = obj->nr_programs;
 	ssize_t idx;
 
-	if (!obj->programs)
+	if (!nr_programs)
 		return NULL;
 
+	if (!p)
+		/* Iter from the beginning */
+		return forward ? &obj->programs[0] :
+			&obj->programs[nr_programs - 1];
+
 	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (p - obj->programs) + i;
+	idx = (p - obj->programs) + (forward ? 1 : -1);
 	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
@@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
-	if (prev == NULL)
-		return obj->programs;
-
 	do {
-		prog = __bpf_program__iter(prog, obj, 1);
+		prog = __bpf_program__iter(prog, obj, true);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;
@@ -2231,14 +2234,8 @@ 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);
+		prog = __bpf_program__iter(prog, obj, false);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;
Stanislav Fomichev Nov. 12, 2018, 11:29 p.m. UTC | #4
On 11/12, Martin Lau wrote:
> On Mon, Nov 12, 2018 at 02:10:11PM -0800, Stanislav Fomichev wrote:
> > 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?
> I think that would work.  The bpf_program__prev() will need the same
> treatment though...
> 
> Here is my mostly untested fix to unblock my other dev works.  It moves
> the very first NULL check back to __bpf_program__iter():
I like your version and it works with my simple flow dissector test :-)
Thanks for spotting and fixing it!


> From de1c89ae1768e756825a6874268b5b1686695c93 Mon Sep 17 00:00:00 2001
> From: Martin KaFai Lau <kafai@fb.com>
> Date: Mon, 12 Nov 2018 14:52:39 -0800
> Subject: [PATCH] bpf: libbpf: Fix bpf_program__next() API
> 
> This patch restores the behavior in
> commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
> such that bpf_program__next() does not return pseudo programs in ".text".
> 
> Fixes: 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e827542ffa3a..a01eb9584e52 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
>  }
>  
>  static struct bpf_program *
> -__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
> +__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
>  {
> +	size_t nr_programs = obj->nr_programs;
>  	ssize_t idx;
>  
> -	if (!obj->programs)
> +	if (!nr_programs)
>  		return NULL;
>  
> +	if (!p)
> +		/* Iter from the beginning */
> +		return forward ? &obj->programs[0] :
> +			&obj->programs[nr_programs - 1];
> +
>  	if (p->obj != obj) {
>  		pr_warning("error: program handler doesn't match object\n");
>  		return NULL;
>  	}
>  
> -	idx = (p - obj->programs) + i;
> +	idx = (p - obj->programs) + (forward ? 1 : -1);
>  	if (idx >= obj->nr_programs || idx < 0)
>  		return NULL;
>  	return &obj->programs[idx];
> @@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
>  {
>  	struct bpf_program *prog = prev;
>  
> -	if (prev == NULL)
> -		return obj->programs;
> -
>  	do {
> -		prog = __bpf_program__iter(prog, obj, 1);
> +		prog = __bpf_program__iter(prog, obj, true);
>  	} while (prog && bpf_program__is_function_storage(prog, obj));
>  
>  	return prog;
> @@ -2231,14 +2234,8 @@ 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);
> +		prog = __bpf_program__iter(prog, obj, false);
>  	} while (prog && bpf_program__is_function_storage(prog, obj));
>  
>  	return prog;
> -- 
> 2.17.1
> 
> > 
> > > >  	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;
Martin KaFai Lau Nov. 12, 2018, 11:41 p.m. UTC | #5
On Mon, Nov 12, 2018 at 03:29:25PM -0800, Stanislav Fomichev wrote:
> On 11/12, Martin Lau wrote:
> > On Mon, Nov 12, 2018 at 02:10:11PM -0800, Stanislav Fomichev wrote:
> > > 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?
> > I think that would work.  The bpf_program__prev() will need the same
> > treatment though...
> > 
> > Here is my mostly untested fix to unblock my other dev works.  It moves
> > the very first NULL check back to __bpf_program__iter():
> I like your version and it works with my simple flow dissector test :-)
Great.  I will send out an offical patch.

> Thanks for spotting and fixing it!
> 
> 
> > From de1c89ae1768e756825a6874268b5b1686695c93 Mon Sep 17 00:00:00 2001
> > From: Martin KaFai Lau <kafai@fb.com>
> > Date: Mon, 12 Nov 2018 14:52:39 -0800
> > Subject: [PATCH] bpf: libbpf: Fix bpf_program__next() API
> > 
> > This patch restores the behavior in
> > commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
> > such that bpf_program__next() does not return pseudo programs in ".text".
> > 
> > Fixes: 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index e827542ffa3a..a01eb9584e52 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
> >  }
> >  
> >  static struct bpf_program *
> > -__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
> > +__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
> >  {
> > +	size_t nr_programs = obj->nr_programs;
> >  	ssize_t idx;
> >  
> > -	if (!obj->programs)
> > +	if (!nr_programs)
> >  		return NULL;
> >  
> > +	if (!p)
> > +		/* Iter from the beginning */
> > +		return forward ? &obj->programs[0] :
> > +			&obj->programs[nr_programs - 1];
> > +
> >  	if (p->obj != obj) {
> >  		pr_warning("error: program handler doesn't match object\n");
> >  		return NULL;
> >  	}
> >  
> > -	idx = (p - obj->programs) + i;
> > +	idx = (p - obj->programs) + (forward ? 1 : -1);
> >  	if (idx >= obj->nr_programs || idx < 0)
> >  		return NULL;
> >  	return &obj->programs[idx];
> > @@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
> >  {
> >  	struct bpf_program *prog = prev;
> >  
> > -	if (prev == NULL)
> > -		return obj->programs;
> > -
> >  	do {
> > -		prog = __bpf_program__iter(prog, obj, 1);
> > +		prog = __bpf_program__iter(prog, obj, true);
> >  	} while (prog && bpf_program__is_function_storage(prog, obj));
> >  
> >  	return prog;
> > @@ -2231,14 +2234,8 @@ 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);
> > +		prog = __bpf_program__iter(prog, obj, false);
> >  	} while (prog && bpf_program__is_function_storage(prog, obj));
> >  
> >  	return prog;
> > -- 
> > 2.17.1
> > 
> > > 
> > > > >  	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;

Patch
diff mbox series

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d6e62e90e8d4..341008f47c8a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1699,6 +1699,34 @@  int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
 	return 0;
 }
 
+int bpf_program__unpin_instance(struct bpf_program *prog, const char *path,
+				int instance)
+{
+	int err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (prog == NULL) {
+		pr_warning("invalid program pointer\n");
+		return -EINVAL;
+	}
+
+	if (instance < 0 || instance >= prog->instances.nr) {
+		pr_warning("invalid prog instance %d of prog %s (max %d)\n",
+			   instance, prog->section_name, prog->instances.nr);
+		return -EINVAL;
+	}
+
+	err = unlink(path);
+	if (err != 0)
+		return -errno;
+	pr_debug("unpinned program '%s'\n", path);
+
+	return 0;
+}
+
 static int make_dir(const char *path)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];
@@ -1737,6 +1765,64 @@  int bpf_program__pin(struct bpf_program *prog, const char *path)
 	if (err)
 		return err;
 
+	for (i = 0; i < prog->instances.nr; i++) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%d", path, i);
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin;
+		}
+
+		err = bpf_program__pin_instance(prog, buf, i);
+		if (err)
+			goto err_unpin;
+	}
+
+	return 0;
+
+err_unpin:
+	for (i = i - 1; i >= 0; i--) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%d", path, i);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_program__unpin_instance(prog, buf, i);
+	}
+
+	rmdir(path);
+
+	return err;
+}
+
+int bpf_program__unpin(struct bpf_program *prog, const char *path)
+{
+	int i, err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (prog == NULL) {
+		pr_warning("invalid program pointer\n");
+		return -EINVAL;
+	}
+
+	if (prog->instances.nr <= 0) {
+		pr_warning("no instances of prog %s to pin\n",
+			   prog->section_name);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < prog->instances.nr; i++) {
 		char buf[PATH_MAX];
 		int len;
@@ -1747,11 +1833,15 @@  int bpf_program__pin(struct bpf_program *prog, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_program__pin_instance(prog, buf, i);
+		err = bpf_program__unpin_instance(prog, buf, i);
 		if (err)
 			return err;
 	}
 
+	err = rmdir(path);
+	if (err)
+		return -errno;
+
 	return 0;
 }
 
@@ -1776,12 +1866,33 @@  int bpf_map__pin(struct bpf_map *map, const char *path)
 	}
 
 	pr_debug("pinned map '%s'\n", path);
+
 	return 0;
 }
 
-int bpf_object__pin(struct bpf_object *obj, const char *path)
+int bpf_map__unpin(struct bpf_map *map, const char *path)
+{
+	int err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (map == NULL) {
+		pr_warning("invalid map pointer\n");
+		return -EINVAL;
+	}
+
+	err = unlink(path);
+	if (err != 0)
+		return -errno;
+	pr_debug("unpinned map '%s'\n", path);
+
+	return 0;
+}
+
+int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 {
-	struct bpf_program *prog;
 	struct bpf_map *map;
 	int err;
 
@@ -1797,6 +1908,53 @@  int bpf_object__pin(struct bpf_object *obj, const char *path)
 	if (err)
 		return err;
 
+	bpf_map__for_each(map, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_maps;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_maps;
+		}
+
+		err = bpf_map__pin(map, buf);
+		if (err)
+			goto err_unpin_maps;
+	}
+
+	return 0;
+
+err_unpin_maps:
+	while ((map = bpf_map__prev(map, obj))) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_map__unpin(map, buf);
+	}
+
+	return err;
+}
+
+int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
+{
+	struct bpf_map *map;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
 	bpf_map__for_each(map, obj) {
 		char buf[PATH_MAX];
 		int len;
@@ -1808,11 +1966,78 @@  int bpf_object__pin(struct bpf_object *obj, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_map__pin(map, buf);
+		err = bpf_map__unpin(map, buf);
 		if (err)
 			return err;
 	}
 
+	return 0;
+}
+
+int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
+{
+	struct bpf_program *prog;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
+	if (!obj->loaded) {
+		pr_warning("object not yet loaded; load it first\n");
+		return -ENOENT;
+	}
+
+	err = make_dir(path);
+	if (err)
+		return err;
+
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_programs;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_programs;
+		}
+
+		err = bpf_program__pin(prog, buf);
+		if (err)
+			goto err_unpin_programs;
+	}
+
+	return 0;
+
+err_unpin_programs:
+	while ((prog = bpf_program__prev(prog, obj))) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_program__unpin(prog, buf);
+	}
+
+	return err;
+}
+
+int bpf_object__unpin_programs(struct bpf_object *obj, const char *path)
+{
+	struct bpf_program *prog;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
 	bpf_object__for_each_program(prog, obj) {
 		char buf[PATH_MAX];
 		int len;
@@ -1824,7 +2049,7 @@  int bpf_object__pin(struct bpf_object *obj, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_program__pin(prog, buf);
+		err = bpf_program__unpin(prog, buf);
 		if (err)
 			return err;
 	}
@@ -1832,6 +2057,23 @@  int bpf_object__pin(struct bpf_object *obj, const char *path)
 	return 0;
 }
 
+int bpf_object__pin(struct bpf_object *obj, const char *path)
+{
+	int err;
+
+	err = bpf_object__pin_maps(obj, path);
+	if (err)
+		return err;
+
+	err = bpf_object__pin_programs(obj, path);
+	if (err) {
+		bpf_object__unpin_maps(obj, path);
+		return err;
+	}
+
+	return 0;
+}
+
 void bpf_object__close(struct bpf_object *obj)
 {
 	size_t i;
@@ -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;
+
 	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;
@@ -2272,10 +2532,10 @@  void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex)
 	map->map_ifindex = ifindex;
 }
 
-struct bpf_map *
-bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
+static struct bpf_map *
+__bpf_map__iter(struct bpf_map *m, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 	struct bpf_map *s, *e;
 
 	if (!obj || !obj->maps)
@@ -2284,21 +2544,39 @@  bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
 	s = obj->maps;
 	e = obj->maps + obj->nr_maps;
 
-	if (prev == NULL)
-		return s;
-
-	if ((prev < s) || (prev >= e)) {
+	if ((m < s) || (m >= e)) {
 		pr_warning("error in %s: map handler doesn't belong to object\n",
 			   __func__);
 		return NULL;
 	}
 
-	idx = (prev - obj->maps) + 1;
-	if (idx >= obj->nr_maps)
+	idx = (m - obj->maps) + i;
+	if (idx >= obj->nr_maps || idx < 0)
 		return NULL;
 	return &obj->maps[idx];
 }
 
+struct bpf_map *
+bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
+{
+	if (prev == NULL)
+		return obj->maps;
+
+	return __bpf_map__iter(prev, obj, 1);
+}
+
+struct bpf_map *
+bpf_map__prev(struct bpf_map *next, struct bpf_object *obj)
+{
+	if (next == NULL) {
+		if (!obj->nr_maps)
+			return NULL;
+		return obj->maps + obj->nr_maps - 1;
+	}
+
+	return __bpf_map__iter(next, obj, -1);
+}
+
 struct bpf_map *
 bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
 {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 1f3468dad8b2..b1686a787102 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -71,6 +71,13 @@  struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
 LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 						      size_t obj_buf_sz,
 						      const char *name);
+LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
+LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
+				      const char *path);
+LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
+					const char *path);
+LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
+					  const char *path);
 LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
 LIBBPF_API void bpf_object__close(struct bpf_object *object);
 
@@ -112,6 +119,9 @@  LIBBPF_API struct bpf_program *bpf_program__next(struct bpf_program *prog,
 	     (pos) != NULL;				\
 	     (pos) = bpf_program__next((pos), (obj)))
 
+LIBBPF_API struct bpf_program *bpf_program__prev(struct bpf_program *prog,
+						 struct bpf_object *obj);
+
 typedef void (*bpf_program_clear_priv_t)(struct bpf_program *,
 					 void *);
 
@@ -131,7 +141,11 @@  LIBBPF_API int bpf_program__fd(struct bpf_program *prog);
 LIBBPF_API int bpf_program__pin_instance(struct bpf_program *prog,
 					 const char *path,
 					 int instance);
+LIBBPF_API int bpf_program__unpin_instance(struct bpf_program *prog,
+					   const char *path,
+					   int instance);
 LIBBPF_API int bpf_program__pin(struct bpf_program *prog, const char *path);
+LIBBPF_API int bpf_program__unpin(struct bpf_program *prog, const char *path);
 LIBBPF_API void bpf_program__unload(struct bpf_program *prog);
 
 struct bpf_insn;
@@ -260,6 +274,9 @@  bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
 	     (pos) != NULL;				\
 	     (pos) = bpf_map__next((pos), (obj)))
 
+LIBBPF_API struct bpf_map *
+bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
+
 LIBBPF_API int bpf_map__fd(struct bpf_map *map);
 LIBBPF_API const struct bpf_map_def *bpf_map__def(struct bpf_map *map);
 LIBBPF_API const char *bpf_map__name(struct bpf_map *map);
@@ -274,6 +291,7 @@  LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
 LIBBPF_API bool bpf_map__is_offload_neutral(struct bpf_map *map);
 LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
 LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path);
+LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
 
 LIBBPF_API long libbpf_get_error(const void *ptr);