From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yonghong Song Subject: Re: [PATCH bpf] tools/bpftool: fix a percpu_array map dump problem Date: Sun, 29 Jul 2018 10:16:21 -0700 Message-ID: <5ee3de47-46a3-7472-d30b-1a366e5ec809@fb.com> References: <20180727231138.3318561-1-yhs@fb.com> <98bd07f3-a132-5477-3108-ac4e0114cd1c@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: To: Daniel Borkmann , , Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:58020 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726438AbeG2Sso (ORCPT ); Sun, 29 Jul 2018 14:48:44 -0400 In-Reply-To: <98bd07f3-a132-5477-3108-ac4e0114cd1c@iogearbox.net> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 7/28/18 12:14 PM, Daniel Borkmann wrote: > On 07/28/2018 01:11 AM, Yonghong Song wrote: >> I hit the following problem when I tried to use bpftool >> to dump a percpu array. >> >> $ sudo ./bpftool map show >> 61: percpu_array name stub flags 0x0 >> key 4B value 4B max_entries 1 memlock 4096B >> ... >> $ sudo ./bpftool map dump id 61 >> bpftool: malloc.c:2406: sysmalloc: Assertion >> `(old_top == initial_top (av) && old_size == 0) || \ >> ((unsigned long) (old_size) >= MINSIZE && \ >> prev_inuse (old_top) && \ >> ((unsigned long) old_end & (pagesize - 1)) == 0)' >> failed. >> Aborted >> >> Further debugging revealed that this is due to >> miscommunication between bpftool and kernel. >> For example, for the above percpu_array with value size of 4B. >> The map info returned to user space has value size of 4B. >> >> In bpftool, the values array for lookup is allocated like: >> info->value_size * get_possible_cpus() = 4 * get_possible_cpus() >> In kernel (kernel/bpf/syscall.c), the values array size is >> rounded up to multiple of 8. >> round_up(map->value_size, 8) * num_possible_cpus() >> = 8 * num_possible_cpus() >> So when kernel copies the values to user buffer, the kernel will >> overwrite beyond user buffer boundary. >> >> This patch fixed the issue by allocating and stepping through >> percpu map value array properly in bpftool. >> >> Fixes: 71bb428fe2c19 ("tools: bpf: add bpftool") >> Signed-off-by: Yonghong Song >> --- >> tools/bpf/bpftool/map.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c >> index 0ee3ba479d87..92bc55f98c4c 100644 >> --- a/tools/bpf/bpftool/map.c >> +++ b/tools/bpf/bpftool/map.c >> @@ -35,6 +35,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -91,7 +92,8 @@ static bool map_is_map_of_progs(__u32 type) >> static void *alloc_value(struct bpf_map_info *info) >> { >> if (map_is_per_cpu(info->type)) >> - return malloc(info->value_size * get_possible_cpus()); >> + return malloc(round_up(info->value_size, 8) * >> + get_possible_cpus()); >> else >> return malloc(info->value_size); >> } >> @@ -273,9 +275,10 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, >> do_dump_btf(&d, info, key, value); >> } >> } else { >> - unsigned int i, n; >> + unsigned int i, n, step; >> >> n = get_possible_cpus(); >> + step = round_up(info->value_size, 8); >> >> jsonw_name(json_wtr, "key"); >> print_hex_data_json(key, info->key_size); >> @@ -288,7 +291,7 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, >> jsonw_int_field(json_wtr, "cpu", i); >> >> jsonw_name(json_wtr, "value"); >> - print_hex_data_json(value + i * info->value_size, >> + print_hex_data_json(value + i * step, >> info->value_size); >> >> jsonw_end_object(json_wtr); > > Fix looks correct to me, but you would also need the same fix in print_entry_plain(), no? Thanks for pointing this out. will submit v2 soon to fix the issue. > > Thanks, > Daniel >