[PATCH bpf v3] tools/bpftool: fix a percpu_array map dump problem

[PATCH bpf v3] tools/bpftool: fix a percpu_array map dump problem

[Yonghong Song]

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 | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Changelogs:
 v2 -> v3:
   . apply on top of bpf tree instead of bpf-next
 v1 -> v2:
   . Added missing fix in function print_entry_plain().

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 097b1a5e046b..f74a8bcbda87 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -36,6 +36,7 @@
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <linux/kernel.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -90,7 +91,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);
 }
@@ -161,9 +163,10 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 		jsonw_name(json_wtr, "value");
 		print_hex_data_json(value, info->value_size);
 	} 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);
@@ -176,7 +179,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);
@@ -207,9 +210,10 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 
 		printf("\n");
 	} else {
-		unsigned int i, n;
+		unsigned int i, n, step;
 
 		n = get_possible_cpus();
+		step = round_up(info->value_size, 8);
 
 		printf("key:\n");
 		fprint_hex(stdout, key, info->key_size, " ");
@@ -217,7 +221,7 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 		for (i = 0; i < n; i++) {
 			printf("value (CPU %02d):%c",
 			       i, info->value_size > 16 ? '\n' : ' ');
-			fprint_hex(stdout, value + i * info->value_size,
+			fprint_hex(stdout, value + i * step,
 				   info->value_size, " ");
 			printf("\n");
 		}
-- 
2.14.3

Re: [PATCH bpf v3] tools/bpftool: fix a percpu_array map dump problem

[Daniel Borkmann]

On 07/30/2018 05:49 PM, 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>

Applied to bpf, thanks Yonghong!