[PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jiri Olsa]

The bpftool interface stays the same, but now it's possible
to run it over BTF raw data, like:

  $ bpftool btf dump file /sys/kernel/btf/vmlinux
  [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
  [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
  [3] CONST '(anon)' type_id=2

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
v2 changes:
 - added is_btf_raw to find out which btf__parse_* function to call
 - changed labels and error propagation in btf__parse_raw 
 - drop the err initialization, which is not needed under this change

 tools/bpf/bpftool/btf.c | 57 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 9a9376d1d3df..a7b8bf233cf5 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -12,6 +12,9 @@
 #include <libbpf.h>
 #include <linux/btf.h>
 #include <linux/hashtable.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "btf.h"
 #include "json_writer.h"
@@ -388,6 +391,54 @@ static int dump_btf_c(const struct btf *btf,
 	return err;
 }
 
+static struct btf *btf__parse_raw(const char *file)
+{
+	struct btf *btf;
+	struct stat st;
+	__u8 *buf;
+	FILE *f;
+
+	if (stat(file, &st))
+		return NULL;
+
+	f = fopen(file, "rb");
+	if (!f)
+		return NULL;
+
+	buf = malloc(st.st_size);
+	if (!buf) {
+		btf = ERR_PTR(-ENOMEM);
+		goto exit_close;
+	}
+
+	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
+		btf = ERR_PTR(-EINVAL);
+		goto exit_free;
+	}
+
+	btf = btf__new(buf, st.st_size);
+
+exit_free:
+	free(buf);
+exit_close:
+	fclose(f);
+	return btf;
+}
+
+static bool is_btf_raw(const char *file)
+{
+	__u16 magic = 0;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	read(fd, &magic, sizeof(magic));
+	close(fd);
+	return magic == BTF_MAGIC;
+}
+
 static int do_dump(int argc, char **argv)
 {
 	struct btf *btf = NULL;
@@ -465,7 +516,11 @@ static int do_dump(int argc, char **argv)
 		}
 		NEXT_ARG();
 	} else if (is_prefix(src, "file")) {
-		btf = btf__parse_elf(*argv, NULL);
+		if (is_btf_raw(*argv))
+			btf = btf__parse_raw(*argv);
+		else
+			btf = btf__parse_elf(*argv, NULL);
+
 		if (IS_ERR(btf)) {
 			err = PTR_ERR(btf);
 			btf = NULL;
-- 
2.21.0

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jiri Olsa]

On Thu, Oct 24, 2019 at 03:23:41PM +0200, Jiri Olsa wrote:
> The bpftool interface stays the same, but now it's possible
> to run it over BTF raw data, like:
> 
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux
>   [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>   [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>   [3] CONST '(anon)' type_id=2
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

ugh, wrong title.. I sent v3 :-\ sry

jirka

> v2 changes:
>  - added is_btf_raw to find out which btf__parse_* function to call
>  - changed labels and error propagation in btf__parse_raw 
>  - drop the err initialization, which is not needed under this change
> 
>  tools/bpf/bpftool/btf.c | 57 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 9a9376d1d3df..a7b8bf233cf5 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -12,6 +12,9 @@
>  #include <libbpf.h>
>  #include <linux/btf.h>
>  #include <linux/hashtable.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>  
>  #include "btf.h"
>  #include "json_writer.h"
> @@ -388,6 +391,54 @@ static int dump_btf_c(const struct btf *btf,
>  	return err;
>  }
>  
> +static struct btf *btf__parse_raw(const char *file)
> +{
> +	struct btf *btf;
> +	struct stat st;
> +	__u8 *buf;
> +	FILE *f;
> +
> +	if (stat(file, &st))
> +		return NULL;
> +
> +	f = fopen(file, "rb");
> +	if (!f)
> +		return NULL;
> +
> +	buf = malloc(st.st_size);
> +	if (!buf) {
> +		btf = ERR_PTR(-ENOMEM);
> +		goto exit_close;
> +	}
> +
> +	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> +		btf = ERR_PTR(-EINVAL);
> +		goto exit_free;
> +	}
> +
> +	btf = btf__new(buf, st.st_size);
> +
> +exit_free:
> +	free(buf);
> +exit_close:
> +	fclose(f);
> +	return btf;
> +}
> +
> +static bool is_btf_raw(const char *file)
> +{
> +	__u16 magic = 0;
> +	int fd;
> +
> +	fd = open(file, O_RDONLY);
> +	if (fd < 0)
> +		return false;
> +
> +	read(fd, &magic, sizeof(magic));
> +	close(fd);
> +	return magic == BTF_MAGIC;
> +}
> +
>  static int do_dump(int argc, char **argv)
>  {
>  	struct btf *btf = NULL;
> @@ -465,7 +516,11 @@ static int do_dump(int argc, char **argv)
>  		}
>  		NEXT_ARG();
>  	} else if (is_prefix(src, "file")) {
> -		btf = btf__parse_elf(*argv, NULL);
> +		if (is_btf_raw(*argv))
> +			btf = btf__parse_raw(*argv);
> +		else
> +			btf = btf__parse_elf(*argv, NULL);
> +
>  		if (IS_ERR(btf)) {
>  			err = PTR_ERR(btf);
>  			btf = NULL;
> -- 
> 2.21.0
> 

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jakub Kicinski]

On Thu, 24 Oct 2019 15:23:41 +0200, Jiri Olsa wrote:
> The bpftool interface stays the same, but now it's possible
> to run it over BTF raw data, like:
> 
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux
>   [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>   [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>   [3] CONST '(anon)' type_id=2

My knee jerk reaction would be to implement a new keyword, like:

$ bpftool btf dump rawfile /sys/kernel/btf/vmlinux

Or such. But perhaps the auto-detection is the standard way of dealing
with different formats in the compiler world. Regardless if anyone has
an opinion one way or the other please share!!

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> v2 changes:
>  - added is_btf_raw to find out which btf__parse_* function to call
>  - changed labels and error propagation in btf__parse_raw 
>  - drop the err initialization, which is not needed under this change

The code looks good, thanks for the changes! One question below..

> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 9a9376d1d3df..a7b8bf233cf5 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c

> +static bool is_btf_raw(const char *file)
> +{
> +	__u16 magic = 0;
> +	int fd;
> +
> +	fd = open(file, O_RDONLY);
> +	if (fd < 0)
> +		return false;
> +
> +	read(fd, &magic, sizeof(magic));
> +	close(fd);
> +	return magic == BTF_MAGIC;

Isn't it suspicious to read() 2 bytes into an u16 and compare to a
constant like endianness doesn't matter? Quick grep doesn't reveal
BTF_MAGIC being endian-aware..

> +}
> +
>  static int do_dump(int argc, char **argv)
>  {
>  	struct btf *btf = NULL;
> @@ -465,7 +516,11 @@ static int do_dump(int argc, char **argv)
>  		}
>  		NEXT_ARG();
>  	} else if (is_prefix(src, "file")) {
> -		btf = btf__parse_elf(*argv, NULL);
> +		if (is_btf_raw(*argv))
> +			btf = btf__parse_raw(*argv);
> +		else
> +			btf = btf__parse_elf(*argv, NULL);
>  		if (IS_ERR(btf)) {
>  			err = PTR_ERR(btf);
>  			btf = NULL;

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Andrii Nakryiko]

On 10/24/19 10:54 AM, Jakub Kicinski wrote:
> On Thu, 24 Oct 2019 15:23:41 +0200, Jiri Olsa wrote:
>> The bpftool interface stays the same, but now it's possible
>> to run it over BTF raw data, like:
>>
>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux
>>    [1] INT '(anon)' size=4 bits_offset=0 nr_bits=32 encoding=(none)
>>    [2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
>>    [3] CONST '(anon)' type_id=2
> 
> My knee jerk reaction would be to implement a new keyword, like:
> 
> $ bpftool btf dump rawfile /sys/kernel/btf/vmlinux
> 
> Or such. But perhaps the auto-detection is the standard way of dealing
> with different formats in the compiler world. Regardless if anyone has
> an opinion one way or the other please share!!

I think auto-detection in this case is easy and reliable, there is no 
way to accidentaly mistake ELF for raw BTF due to different magics. 
Besides that, it's so much better usability for users to not have to 
care. Plus, no need to extend shell auto-completion script :-P

> 
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> ---
>> v2 changes:
>>   - added is_btf_raw to find out which btf__parse_* function to call
>>   - changed labels and error propagation in btf__parse_raw
>>   - drop the err initialization, which is not needed under this change
> 
> The code looks good, thanks for the changes! One question below..
> 
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 9a9376d1d3df..a7b8bf233cf5 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
> 
>> +static bool is_btf_raw(const char *file)
>> +{
>> +	__u16 magic = 0;
>> +	int fd;
>> +
>> +	fd = open(file, O_RDONLY);
>> +	if (fd < 0)
>> +		return false;
>> +
>> +	read(fd, &magic, sizeof(magic));
>> +	close(fd);
>> +	return magic == BTF_MAGIC;
> 
> Isn't it suspicious to read() 2 bytes into an u16 and compare to a
> constant like endianness doesn't matter? Quick grep doesn't reveal
> BTF_MAGIC being endian-aware..

Right now we support only loading BTF in native endianness, so I think 
this should do. If we ever add ability to load non-native endianness, 
then we'll have to adjust this.

> 
>> +}
>> +
>>   static int do_dump(int argc, char **argv)
>>   {
>>   	struct btf *btf = NULL;
>> @@ -465,7 +516,11 @@ static int do_dump(int argc, char **argv)
>>   		}
>>   		NEXT_ARG();
>>   	} else if (is_prefix(src, "file")) {
>> -		btf = btf__parse_elf(*argv, NULL);
>> +		if (is_btf_raw(*argv))
>> +			btf = btf__parse_raw(*argv);
>> +		else
>> +			btf = btf__parse_elf(*argv, NULL);
>>   		if (IS_ERR(btf)) {
>>   			err = PTR_ERR(btf);
>>   			btf = NULL;
> 

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jakub Kicinski]

On Fri, 25 Oct 2019 05:01:17 +0000, Andrii Nakryiko wrote:
> >> +static bool is_btf_raw(const char *file)
> >> +{
> >> +	__u16 magic = 0;
> >> +	int fd;
> >> +
> >> +	fd = open(file, O_RDONLY);
> >> +	if (fd < 0)
> >> +		return false;
> >> +
> >> +	read(fd, &magic, sizeof(magic));
> >> +	close(fd);
> >> +	return magic == BTF_MAGIC;  
> > 
> > Isn't it suspicious to read() 2 bytes into an u16 and compare to a
> > constant like endianness doesn't matter? Quick grep doesn't reveal
> > BTF_MAGIC being endian-aware..  
> 
> Right now we support only loading BTF in native endianness, so I think 
> this should do. If we ever add ability to load non-native endianness, 
> then we'll have to adjust this.

This doesn't do native endianness, this does LE-only. It will not work
on BE machines.

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jiri Olsa]

On Fri, Oct 25, 2019 at 09:31:16AM -0700, Jakub Kicinski wrote:
> On Fri, 25 Oct 2019 05:01:17 +0000, Andrii Nakryiko wrote:
> > >> +static bool is_btf_raw(const char *file)
> > >> +{
> > >> +	__u16 magic = 0;
> > >> +	int fd;
> > >> +
> > >> +	fd = open(file, O_RDONLY);
> > >> +	if (fd < 0)
> > >> +		return false;
> > >> +
> > >> +	read(fd, &magic, sizeof(magic));
> > >> +	close(fd);
> > >> +	return magic == BTF_MAGIC;  
> > > 
> > > Isn't it suspicious to read() 2 bytes into an u16 and compare to a
> > > constant like endianness doesn't matter? Quick grep doesn't reveal
> > > BTF_MAGIC being endian-aware..  
> > 
> > Right now we support only loading BTF in native endianness, so I think 
> > this should do. If we ever add ability to load non-native endianness, 
> > then we'll have to adjust this.
> 
> This doesn't do native endianness, this does LE-only. It will not work
> on BE machines.

hum, let me try.. I thought it would

jirka

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Andrii Nakryiko]

On Fri, Oct 25, 2019 at 9:31 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 25 Oct 2019 05:01:17 +0000, Andrii Nakryiko wrote:
> > >> +static bool is_btf_raw(const char *file)
> > >> +{
> > >> +  __u16 magic = 0;
> > >> +  int fd;
> > >> +
> > >> +  fd = open(file, O_RDONLY);
> > >> +  if (fd < 0)
> > >> +          return false;
> > >> +
> > >> +  read(fd, &magic, sizeof(magic));
> > >> +  close(fd);
> > >> +  return magic == BTF_MAGIC;
> > >
> > > Isn't it suspicious to read() 2 bytes into an u16 and compare to a
> > > constant like endianness doesn't matter? Quick grep doesn't reveal
> > > BTF_MAGIC being endian-aware..
> >
> > Right now we support only loading BTF in native endianness, so I think
> > this should do. If we ever add ability to load non-native endianness,
> > then we'll have to adjust this.
>
> This doesn't do native endianness, this does LE-only. It will not work
> on BE machines.

How is this LE-only? You have 2 first bytes in BE-encoding on BE
machines and in LE-encoding on LE machines. You read those two bytes
as is into u16, then do comparison to u16. Given all of that is
supposed to be in native encoding, this will work. What am I missing?

Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

[Jakub Kicinski]

On Fri, 25 Oct 2019 09:53:07 -0700, Andrii Nakryiko wrote:
> On Fri, Oct 25, 2019 at 9:31 AM Jakub Kicinski wrote:
> > On Fri, 25 Oct 2019 05:01:17 +0000, Andrii Nakryiko wrote:  
> > > >> +static bool is_btf_raw(const char *file)
> > > >> +{
> > > >> +  __u16 magic = 0;
> > > >> +  int fd;
> > > >> +
> > > >> +  fd = open(file, O_RDONLY);
> > > >> +  if (fd < 0)
> > > >> +          return false;
> > > >> +
> > > >> +  read(fd, &magic, sizeof(magic));
> > > >> +  close(fd);
> > > >> +  return magic == BTF_MAGIC;  
> > > >
> > > > Isn't it suspicious to read() 2 bytes into an u16 and compare to a
> > > > constant like endianness doesn't matter? Quick grep doesn't reveal
> > > > BTF_MAGIC being endian-aware..  
> > >
> > > Right now we support only loading BTF in native endianness, so I think
> > > this should do. If we ever add ability to load non-native endianness,
> > > then we'll have to adjust this.  
> >
> > This doesn't do native endianness, this does LE-only. It will not work
> > on BE machines.  
> 
> How is this LE-only? You have 2 first bytes in BE-encoding on BE
> machines and in LE-encoding on LE machines. You read those two bytes
> as is into u16, then do comparison to u16. Given all of that is
> supposed to be in native encoding, this will work. What am I missing?

I see so the on-disk format depends on the endianness of the writer?
You write it broken and therefore you can also read it broken.
And cross compilation etc. is, I presume, on the TODO list?

Got it.