| Message ID | 20180727231138.3318561-1-yhs@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] tools/bpftool: fix a percpu_array map dump problem | expand |
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 <yhs@fb.com> > --- > 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 <errno.h> > #include <fcntl.h> > #include <linux/err.h> > +#include <linux/kernel.h> > #include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> > @@ -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, Daniel
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 <yhs@fb.com> >> --- >> 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 <errno.h> >> #include <fcntl.h> >> #include <linux/err.h> >> +#include <linux/kernel.h> >> #include <stdbool.h> >> #include <stdio.h> >> #include <stdlib.h> >> @@ -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 >
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 <errno.h> #include <fcntl.h> #include <linux/err.h> +#include <linux/kernel.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> @@ -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);
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 <yhs@fb.com> --- tools/bpf/bpftool/map.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)