[bpf-next,2/3] bpftool: match programs by name
diff mbox series

Message ID 1e3ede4f901a36af342e71bc4fdd2b27fbf9a418.1575991886.git.paul.chaignon@orange.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • bpftool: match programs and maps by names
Related show

Commit Message

Paul Chaignon Dec. 10, 2019, 4:06 p.m. UTC
When working with frequently modified BPF programs, both the ID and the
tag may change.  bpftool currently doesn't provide a "stable" way to match
such programs.

This patch implements lookup by name for programs.  The show and dump
commands will return all programs with the given name, whereas other
commands will error out if several programs have the same name.

Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
---
 .../bpf/bpftool/Documentation/bpftool-map.rst |  2 +-
 .../bpftool/Documentation/bpftool-prog.rst    | 12 +++++-----
 tools/bpf/bpftool/bash-completion/bpftool     | 22 ++++++++++++-----
 tools/bpf/bpftool/main.h                      |  2 +-
 tools/bpf/bpftool/prog.c                      | 24 +++++++++++++++----
 5 files changed, 43 insertions(+), 19 deletions(-)

Comments

Quentin Monnet Dec. 10, 2019, 5:29 p.m. UTC | #1
2019-12-10 17:06 UTC+0100 ~ Paul Chaignon <paul.chaignon@orange.com>
> When working with frequently modified BPF programs, both the ID and the
> tag may change.  bpftool currently doesn't provide a "stable" way to match
> such programs.
> 
> This patch implements lookup by name for programs.  The show and dump
> commands will return all programs with the given name, whereas other
> commands will error out if several programs have the same name.
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>

Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

Thanks!
Quentin
Jakub Kicinski Dec. 10, 2019, 9:04 p.m. UTC | #2
On Tue, 10 Dec 2019 17:06:42 +0100, Paul Chaignon wrote:
> When working with frequently modified BPF programs, both the ID and the
> tag may change.  bpftool currently doesn't provide a "stable" way to match
> such programs.
> 
> This patch implements lookup by name for programs.  The show and dump
> commands will return all programs with the given name, whereas other
> commands will error out if several programs have the same name.
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>

> @@ -164,7 +165,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
>  		}
>  		return 1;
>  	} else if (is_prefix(**argv, "tag")) {
> -		unsigned char tag[BPF_TAG_SIZE];
> +		char tag[BPF_TAG_SIZE];

Perhaps better to change the argument to prog_fd_by_nametag() to void *?

>  
>  		NEXT_ARGP();
>  
> @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
>  		}
>  		NEXT_ARGP();
>  
> -		return prog_fd_by_tag(tag, fds);
> +		return prog_fd_by_nametag(tag, fds, true);
> +	} else if (is_prefix(**argv, "name")) {
> +		char *name;
> +
> +		NEXT_ARGP();
> +
> +		name = **argv;
> +		if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {

Is this needed? strncmp will simply never match, is it preferred to
hard error?

> +			p_err("can't parse name");
> +			return -1;
> +		}
> +		NEXT_ARGP();
> +
> +		return prog_fd_by_nametag(name, fds, false);
>  	} else if (is_prefix(**argv, "pinned")) {
>  		char *path;
>  
> @@ -191,7 +205,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
>  		return 1;
>  	}
>  
> -	p_err("expected 'id', 'tag' or 'pinned', got: '%s'?", **argv);
> +	p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
>  	return -1;
>  }
>
Paul Chaignon Dec. 13, 2019, 12:40 p.m. UTC | #3
On Tue, Dec 10, 2019 at 01:04:13PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 17:06:42 +0100, Paul Chaignon wrote:
> > When working with frequently modified BPF programs, both the ID and the
> > tag may change.  bpftool currently doesn't provide a "stable" way to match
> > such programs.
> > 
> > This patch implements lookup by name for programs.  The show and dump
> > commands will return all programs with the given name, whereas other
> > commands will error out if several programs have the same name.
> > 
> > Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> 
> > @@ -164,7 +165,7 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> >  		}
> >  		return 1;
> >  	} else if (is_prefix(**argv, "tag")) {
> > -		unsigned char tag[BPF_TAG_SIZE];
> > +		char tag[BPF_TAG_SIZE];
> 
> Perhaps better to change the argument to prog_fd_by_nametag() to void *?
> 
> >  
> >  		NEXT_ARGP();
> >  
> > @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> >  		}
> >  		NEXT_ARGP();
> >  
> > -		return prog_fd_by_tag(tag, fds);
> > +		return prog_fd_by_nametag(tag, fds, true);
> > +	} else if (is_prefix(**argv, "name")) {
> > +		char *name;
> > +
> > +		NEXT_ARGP();
> > +
> > +		name = **argv;
> > +		if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
> 
> Is this needed? strncmp will simply never match, is it preferred to
> hard error?

I tried to follow the fail-early pattern of lookups by tag above.  I do
like that there's a different error message for a longer than expected
name.  Since libbpf silently truncates names, typing a longer name is
not uncommon.

[...]

Paul
Jakub Kicinski Dec. 13, 2019, 5:56 p.m. UTC | #4
On Fri, 13 Dec 2019 13:40:38 +0100, Paul Chaignon wrote:
> > > @@ -176,7 +177,20 @@ prog_parse_fds(int *argc, char ***argv, int *fds)
> > >  		}
> > >  		NEXT_ARGP();
> > >  
> > > -		return prog_fd_by_tag(tag, fds);
> > > +		return prog_fd_by_nametag(tag, fds, true);
> > > +	} else if (is_prefix(**argv, "name")) {
> > > +		char *name;
> > > +
> > > +		NEXT_ARGP();
> > > +
> > > +		name = **argv;
> > > +		if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {  
> > 
> > Is this needed? strncmp will simply never match, is it preferred to
> > hard error?  
> 
> I tried to follow the fail-early pattern of lookups by tag above.  

Right although tag does a scanf and if we didn't scan all letters 
we'd use uninit memory.

> I do like that there's a different error message for a longer than
> expected name.  Since libbpf silently truncates names, typing a
> longer name is not uncommon.

Ugh, I didn't realize libbpf truncates names. Okay, let's keep the
error for now so we can switch to truncation if users complain.

Patch
diff mbox series

diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 1c0f7146aab0..90d848b5b7d3 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -41,7 +41,7 @@  MAP COMMANDS
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
 |	*DATA* := { [**hex**] *BYTES* }
-|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
 |	*VALUE* := { *DATA* | *MAP* | *PROG* }
 |	*UPDATE_FLAGS* := { **any** | **exist** | **noexist** }
 |	*TYPE* := { **hash** | **array** | **prog_array** | **perf_event_array** | **percpu_hash**
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index d377d0cb7923..64ddf8a4c518 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -33,7 +33,7 @@  PROG COMMANDS
 |	**bpftool** **prog help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
-|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* | **name** *PROG_NAME* }
 |	*TYPE* := {
 |		**socket** | **kprobe** | **kretprobe** | **classifier** | **action** |
 |		**tracepoint** | **raw_tracepoint** | **xdp** | **perf_event** | **cgroup/skb** |
@@ -55,8 +55,8 @@  DESCRIPTION
 		  Show information about loaded programs.  If *PROG* is
 		  specified show information only about given programs,
 		  otherwise list all programs currently loaded on the system.
-		  In case of **tag**, *PROG* may match several programs which
-		  will all be shown.
+		  In case of **tag** or **name**, *PROG* may match several
+		  programs which will all be shown.
 
 		  Output will start with program ID followed by program type and
 		  zero or more named attributes (depending on kernel version).
@@ -75,9 +75,9 @@  DESCRIPTION
 		  output in human-readable format. In this case, **opcodes**
 		  controls if raw opcodes should be printed as well.
 
-		  In case of **tag**, *PROG* may match several programs which
-		  will all be dumped.  However, if **file** or **visual** is
-		  specified, *PROG* must match a single program.
+		  In case of **tag** or **name**, *PROG* may match several
+		  programs which will all be dumped.  However, if **file** or
+		  **visual** is specified, *PROG* must match a single program.
 
 		  If **file** is specified, the binary image will instead be
 		  written to *FILE*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 70493a6da206..05b5be4a6ef9 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -71,6 +71,12 @@  _bpftool_get_prog_tags()
         command sed -n 's/.*"tag": "\(.*\)",$/\1/p' )" -- "$cur" ) )
 }
 
+_bpftool_get_prog_names()
+{
+    COMPREPLY+=( $( compgen -W "$( bpftool -jp prog 2>&1 | \
+        command sed -n 's/.*"name": "\(.*\)",$/\1/p' )" -- "$cur" ) )
+}
+
 _bpftool_get_btf_ids()
 {
     COMPREPLY+=( $( compgen -W "$( bpftool -jp btf 2>&1 | \
@@ -201,6 +207,10 @@  _bpftool()
             _bpftool_get_prog_tags
             return 0
             ;;
+        name)
+            _bpftool_get_prog_names
+            return 0
+            ;;
         dev)
             _sysfs_get_netdevs
             return 0
@@ -263,7 +273,7 @@  _bpftool()
                     ;;
             esac
 
-            local PROG_TYPE='id pinned tag'
+            local PROG_TYPE='id pinned tag name'
             local MAP_TYPE='id pinned'
             case $command in
                 show|list)
@@ -559,7 +569,7 @@  _bpftool()
                                     return 0
                                     ;;
                                 prog_array)
-                                    local PROG_TYPE='id pinned tag'
+                                    local PROG_TYPE='id pinned tag name'
                                     COMPREPLY+=( $( compgen -W "$PROG_TYPE" \
                                         -- "$cur" ) )
                                     return 0
@@ -644,7 +654,7 @@  _bpftool()
             esac
             ;;
         btf)
-            local PROG_TYPE='id pinned tag'
+            local PROG_TYPE='id pinned tag name'
             local MAP_TYPE='id pinned'
             case $command in
                 dump)
@@ -735,7 +745,7 @@  _bpftool()
                         connect6 sendmsg4 sendmsg6 recvmsg4 recvmsg6 sysctl \
                         getsockopt setsockopt'
                     local ATTACH_FLAGS='multi override'
-                    local PROG_TYPE='id pinned tag'
+                    local PROG_TYPE='id pinned tag name'
                     case $prev in
                         $command)
                             _filedir
@@ -760,7 +770,7 @@  _bpftool()
                             elif [[ "$command" == "attach" ]]; then
                                 # We have an attach type on the command line,
                                 # but it is not the previous word, or
-                                # "id|pinned|tag" (we already checked for
+                                # "id|pinned|tag|name" (we already checked for
                                 # that). This should only leave the case when
                                 # we need attach flags for "attach" commamnd.
                                 _bpftool_one_of_list "$ATTACH_FLAGS"
@@ -786,7 +796,7 @@  _bpftool()
             esac
             ;;
         net)
-            local PROG_TYPE='id pinned tag'
+            local PROG_TYPE='id pinned tag name'
             local ATTACH_TYPES='xdp xdpgeneric xdpdrv xdpoffload'
             case $command in
                 show|list)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 2899095f8254..a7ead7bb9447 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -42,7 +42,7 @@ 
 #define BPF_TAG_FMT	"%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
 #define HELP_SPEC_PROGRAM						\
-	"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
+	"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG | name PROG_NAME }"
 #define HELP_SPEC_OPTIONS						\
 	"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n"	\
 	"\t            {-m|--mapcompat} | {-n|--nomount} }"
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index ca4278269e73..8d3eedf7734d 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -83,7 +83,7 @@  static void print_boot_time(__u64 nsecs, char *buf, unsigned int size)
 }
 
 static int
-prog_fd_by_tag(unsigned char *tag, int *fds)
+prog_fd_by_nametag(char *nametag, int *fds, bool tag)
 {
 	unsigned int id = 0;
 	int fd, nb_fds = 0;
@@ -117,7 +117,8 @@  prog_fd_by_tag(unsigned char *tag, int *fds)
 			goto err_close_fd;
 		}
 
-		if (memcmp(tag, info.tag, BPF_TAG_SIZE)) {
+		if ((tag && memcmp(nametag, info.tag, BPF_TAG_SIZE)) ||
+		    (!tag && strncmp(nametag, info.name, BPF_OBJ_NAME_LEN))) {
 			close(fd);
 			continue;
 		}
@@ -164,7 +165,7 @@  prog_parse_fds(int *argc, char ***argv, int *fds)
 		}
 		return 1;
 	} else if (is_prefix(**argv, "tag")) {
-		unsigned char tag[BPF_TAG_SIZE];
+		char tag[BPF_TAG_SIZE];
 
 		NEXT_ARGP();
 
@@ -176,7 +177,20 @@  prog_parse_fds(int *argc, char ***argv, int *fds)
 		}
 		NEXT_ARGP();
 
-		return prog_fd_by_tag(tag, fds);
+		return prog_fd_by_nametag(tag, fds, true);
+	} else if (is_prefix(**argv, "name")) {
+		char *name;
+
+		NEXT_ARGP();
+
+		name = **argv;
+		if (strlen(name) > BPF_OBJ_NAME_LEN - 1) {
+			p_err("can't parse name");
+			return -1;
+		}
+		NEXT_ARGP();
+
+		return prog_fd_by_nametag(name, fds, false);
 	} else if (is_prefix(**argv, "pinned")) {
 		char *path;
 
@@ -191,7 +205,7 @@  prog_parse_fds(int *argc, char ***argv, int *fds)
 		return 1;
 	}
 
-	p_err("expected 'id', 'tag' or 'pinned', got: '%s'?", **argv);
+	p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
 	return -1;
 }