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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 25 Oct 2019 05:01:17 +0000
From:   Andrii Nakryiko <andriin@...com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jiri Olsa <jolsa@...nel.org>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Yonghong Song <yhs@...com>, Martin Lau <kafai@...com>,
        "andrii.nakryiko@...il.com" <andrii.nakryiko@...il.com>
Subject: Re: [PATCHv2] bpftool: Try to read btf as raw data if elf read fails

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@...nel.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;
>