[RFC v2 bpf-next 0/5] Error handling when map lookup isn't supported

[RFC v2 bpf-next 0/5] Error handling when map lookup isn't supported

[Prashant Bhole]

Currently when map a lookup fails, user space API can not make any
distinction whether given key was not found or lookup is not supported
by particular map.

In this series we modify return value of maps which do not support
lookup. Lookup on such map implementation will return -EOPNOTSUPP.
bpf() syscall with BPF_MAP_LOOKUP_ELEM command will set EOPNOTSUPP
errno. We also handle this error in bpftool to print appropriate
message.

Patch 1: adds handling of BPF_MAP_LOOKUP ELEM command of bpf syscall
such that errno will set to EOPNOTSUPP when map doesn't support lookup

Patch 2: Modifies the return value of map_lookup_elem() to EOPNOTSUPP
for maps which do not support lookup

Patch 3: Splits do_dump() in bpftool/map.c. Element printing code is
moved out into new function dump_map_elem(). This was done in order to
reduce deep indentation and accomodate further changes.

Patch 4: Changes in bpftool to print strerror() message when lookup
error is occured. This will result in appropriate message like
"Operation not supported" when map doesn't support lookup.

Patch 5: Added verifier tests to check whether verifier rejects call 
to bpf_map_lookup_elem from bpf program. For all map types those
do not support map lookup.

v2: 
- bpftool: all nit-pick fixes pointed out by Jakub
- bpftool: removed usage of error strings. Now using strerror(),
  suggested by Jakub
- added tests in verifier_tests, suggested by Alexei


Prashant Bhole (5):
  bpf: error handling when map_lookup_elem isn't supported
  bpf: return EOPNOTSUPP when map lookup isn't supported
  tools/bpf: bpftool, split the function do_dump()
  tools/bpf: bpftool, print strerror when map lookup error occurs
  selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog

 kernel/bpf/arraymap.c                       |   2 +-
 kernel/bpf/sockmap.c                        |   2 +-
 kernel/bpf/stackmap.c                       |   2 +-
 kernel/bpf/syscall.c                        |   9 +-
 kernel/bpf/xskmap.c                         |   2 +-
 tools/bpf/bpftool/map.c                     | 102 +++++++++++------
 tools/testing/selftests/bpf/test_verifier.c | 121 +++++++++++++++++++-
 7 files changed, 199 insertions(+), 41 deletions(-)

-- 
2.17.1

[RFC v2 bpf-next 1/5] bpf: error handling when map_lookup_elem isn't supported

[Prashant Bhole]

The error value returned by map_lookup_elem doesn't differentiate
whether lookup was failed because of invalid key or lookup is not
supported.

Lets add handling for -EOPNOTSUPP return value of map_lookup_elem()
method of map, with expectation from map's implementation that it
should return -EOPNOTSUPP if lookup is not supported.

The errno for bpf syscall for BPF_MAP_LOOKUP_ELEM command will be set
to EOPNOTSUPP if map lookup is not supported.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 kernel/bpf/syscall.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5742df21598c..4f416234251f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -719,10 +719,15 @@ static int map_lookup_elem(union bpf_attr *attr)
 	} else {
 		rcu_read_lock();
 		ptr = map->ops->map_lookup_elem(map, key);
-		if (ptr)
+		if (IS_ERR(ptr)) {
+			err = PTR_ERR(ptr);
+		} else if (!ptr) {
+			err = -ENOENT;
+		} else {
+			err = 0;
 			memcpy(value, ptr, value_size);
+		}
 		rcu_read_unlock();
-		err = ptr ? 0 : -ENOENT;
 	}
 
 	if (err)
-- 
2.17.1

[RFC v2 bpf-next 2/5] bpf: return EOPNOTSUPP when map lookup isn't supported

[Prashant Bhole]

Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
map types:
- BPF_MAP_TYPE_PROG_ARRAY
- BPF_MAP_TYPE_STACK_TRACE
- BPF_MAP_TYPE_XSKMAP
- BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 kernel/bpf/arraymap.c | 2 +-
 kernel/bpf/sockmap.c  | 2 +-
 kernel/bpf/stackmap.c | 2 +-
 kernel/bpf/xskmap.c   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index dded84cbe814..24583da9ffd1 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
 
 static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 /* only called from syscall */
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index d37a1a0a6e1e..5d0677d808ae 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2096,7 +2096,7 @@ int sockmap_get_from_fd(const union bpf_attr *attr, int type,
 
 static void *sock_map_lookup(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 static int sock_map_update_elem(struct bpf_map *map,
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 8061a439ef18..b2ade10f7ec3 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -505,7 +505,7 @@ const struct bpf_func_proto bpf_get_stack_proto = {
 /* Called from eBPF program */
 static void *stack_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 /* Called from syscall */
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index 9f8463afda9c..ef0b7b6ef8a5 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -154,7 +154,7 @@ void __xsk_map_flush(struct bpf_map *map)
 
 static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
 {
-	return NULL;
+	return ERR_PTR(-EOPNOTSUPP);
 }
 
 static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
-- 
2.17.1

[RFC v2 bpf-next 3/5] tools/bpf: bpftool, split the function do_dump()

[Prashant Bhole]

do_dump() function in bpftool/map.c has deep indentations. In order
to reduce deep indent, let's move element printing code out of
do_dump() into dump_map_elem() function.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/bpf/bpftool/map.c | 83 ++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 6003e9598973..28d365435fea 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -658,6 +658,54 @@ static int do_show(int argc, char **argv)
 	return errno == ENOENT ? 0 : -1;
 }
 
+static int dump_map_elem(int fd, void *key, void *value,
+			 struct bpf_map_info *map_info, struct btf *btf,
+			 json_writer_t *btf_wtr)
+{
+	int num_elems = 0;
+
+	if (!bpf_map_lookup_elem(fd, key, value)) {
+		if (json_output) {
+			print_entry_json(map_info, key, value, btf);
+		} else {
+			if (btf) {
+				struct btf_dumper d = {
+					.btf = btf,
+					.jw = btf_wtr,
+					.is_plain_text = true,
+				};
+
+				do_dump_btf(&d, map_info, key, value);
+			} else {
+				print_entry_plain(map_info, key, value);
+			}
+			num_elems++;
+		}
+		return num_elems;
+	}
+
+	/* lookup error handling */
+	if (map_is_map_of_maps(map_info->type) ||
+	    map_is_map_of_progs(map_info->type))
+		return 0;
+
+	if (json_output) {
+		jsonw_name(json_wtr, "key");
+		print_hex_data_json(key, map_info->key_size);
+		jsonw_name(json_wtr, "value");
+		jsonw_start_object(json_wtr);
+		jsonw_string_field(json_wtr, "error",
+				   "can't lookup element");
+		jsonw_end_object(json_wtr);
+	} else {
+		p_info("can't lookup element with key: ");
+		fprint_hex(stderr, key, map_info->key_size, " ");
+		fprintf(stderr, "\n");
+	}
+
+	return 0;
+}
+
 static int do_dump(int argc, char **argv)
 {
 	struct bpf_map_info info = {};
@@ -713,40 +761,7 @@ static int do_dump(int argc, char **argv)
 				err = 0;
 			break;
 		}
-
-		if (!bpf_map_lookup_elem(fd, key, value)) {
-			if (json_output)
-				print_entry_json(&info, key, value, btf);
-			else
-				if (btf) {
-					struct btf_dumper d = {
-						.btf = btf,
-						.jw = btf_wtr,
-						.is_plain_text = true,
-					};
-
-					do_dump_btf(&d, &info, key, value);
-				} else {
-					print_entry_plain(&info, key, value);
-				}
-			num_elems++;
-		} else if (!map_is_map_of_maps(info.type) &&
-			   !map_is_map_of_progs(info.type)) {
-			if (json_output) {
-				jsonw_name(json_wtr, "key");
-				print_hex_data_json(key, info.key_size);
-				jsonw_name(json_wtr, "value");
-				jsonw_start_object(json_wtr);
-				jsonw_string_field(json_wtr, "error",
-						   "can't lookup element");
-				jsonw_end_object(json_wtr);
-			} else {
-				p_info("can't lookup element with key: ");
-				fprint_hex(stderr, key, info.key_size, " ");
-				fprintf(stderr, "\n");
-			}
-		}
-
+		num_elems += dump_map_elem(fd, key, value, &info, btf, btf_wtr);
 		prev_key = key;
 	}
 
-- 
2.17.1

[RFC v2 bpf-next 4/5] tools/bpf: bpftool, print strerror when map lookup error occurs

[Prashant Bhole]

Since map lookup error can be ENOENT or EOPNOTSUPP, let's print
strerror() as error message in normal and JSON output.

This patch adds helper function print_entry_error() to print
entry from lookup error occurs

Example: Following example dumps a map which does not support lookup.

Output before:
root# bpftool map -jp dump id 40
[
    "key": ["0x0a","0x00","0x00","0x00"
    ],
    "value": {
        "error": "can\'t lookup element"
    },
    "key": ["0x0b","0x00","0x00","0x00"
    ],
    "value": {
        "error": "can\'t lookup element"
    }
]

root# bpftool map dump id 40
can't lookup element with key:
0a 00 00 00
can't lookup element with key:
0b 00 00 00
Found 0 elements

Output after changes:
root# bpftool map dump -jp  id 45
[
    "key": ["0x0a","0x00","0x00","0x00"
    ],
    "value": {
        "error": "Operation not supported"
    },
    "key": ["0x0b","0x00","0x00","0x00"
    ],
    "value": {
        "error": "Operation not supported"
    }
]

root# bpftool map dump id 45
key:
0a 00 00 00
value:
Operation not supported
key:
0b 00 00 00
value:
Operation not supported
Found 0 elements

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/bpf/bpftool/map.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 28d365435fea..9f5de48f8a99 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -336,6 +336,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
 	jsonw_end_object(json_wtr);
 }
 
+static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
+			      const char *value)
+{
+	int value_size = strlen(value);
+	bool single_line, break_names;
+
+	break_names = info->key_size > 16 || value_size > 16;
+	single_line = info->key_size + value_size <= 24 && !break_names;
+
+	printf("key:%c", break_names ? '\n' : ' ');
+	fprint_hex(stdout, key, info->key_size, " ");
+
+	printf(single_line ? "  " : "\n");
+
+	printf("value:%c%s", break_names ? '\n' : ' ', value);
+
+	printf("\n");
+}
+
 static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
 			      unsigned char *value)
 {
@@ -663,6 +682,7 @@ static int dump_map_elem(int fd, void *key, void *value,
 			 json_writer_t *btf_wtr)
 {
 	int num_elems = 0;
+	int lookup_errno;
 
 	if (!bpf_map_lookup_elem(fd, key, value)) {
 		if (json_output) {
@@ -685,6 +705,8 @@ static int dump_map_elem(int fd, void *key, void *value,
 	}
 
 	/* lookup error handling */
+	lookup_errno = errno;
+
 	if (map_is_map_of_maps(map_info->type) ||
 	    map_is_map_of_progs(map_info->type))
 		return 0;
@@ -694,13 +716,10 @@ static int dump_map_elem(int fd, void *key, void *value,
 		print_hex_data_json(key, map_info->key_size);
 		jsonw_name(json_wtr, "value");
 		jsonw_start_object(json_wtr);
-		jsonw_string_field(json_wtr, "error",
-				   "can't lookup element");
+		jsonw_string_field(json_wtr, "error", strerror(lookup_errno));
 		jsonw_end_object(json_wtr);
 	} else {
-		p_info("can't lookup element with key: ");
-		fprint_hex(stderr, key, map_info->key_size, " ");
-		fprintf(stderr, "\n");
+		print_entry_error(map_info, key, strerror(lookup_errno));
 	}
 
 	return 0;
-- 
2.17.1

[RFC v2 bpf-next 5/5] selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog

[Prashant Bhole]

map_lookup_elem isn't supported by certain map types like:
- BPF_MAP_TYPE_PROG_ARRAY
- BPF_MAP_TYPE_STACK_TRACE
- BPF_MAP_TYPE_XSKMAP
- BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
Let's add verfier tests to check whether verifier prevents
bpf_map_lookup_elem call on above programs from bpf program.

Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
 tools/testing/selftests/bpf/test_verifier.c | 121 +++++++++++++++++++-
 1 file changed, 120 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c7d25f23baf9..afa7e67f66e4 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -47,7 +47,7 @@
 
 #define MAX_INSNS	BPF_MAXINSNS
 #define MAX_FIXUPS	8
-#define MAX_NR_MAPS	8
+#define MAX_NR_MAPS	13
 #define POINTER_VALUE	0xcafe4all
 #define TEST_DATA_LEN	64
 
@@ -64,6 +64,10 @@ struct bpf_test {
 	int fixup_map2[MAX_FIXUPS];
 	int fixup_map3[MAX_FIXUPS];
 	int fixup_map4[MAX_FIXUPS];
+	int fixup_map5[MAX_FIXUPS];
+	int fixup_map6[MAX_FIXUPS];
+	int fixup_map7[MAX_FIXUPS];
+	int fixup_map8[MAX_FIXUPS];
 	int fixup_prog1[MAX_FIXUPS];
 	int fixup_prog2[MAX_FIXUPS];
 	int fixup_map_in_map[MAX_FIXUPS];
@@ -4391,6 +4395,85 @@ static struct bpf_test tests[] = {
 		.errstr = "invalid access to packet",
 		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	},
+	{
+		"prevent map lookup in sockmap",
+		.insns = {
+			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+			BPF_LD_MAP_FD(BPF_REG_1, 0),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_map_lookup_elem),
+			BPF_EXIT_INSN(),
+		},
+		.fixup_map5 = { 3 },
+		.result = REJECT,
+		.errstr = "cannot pass map_type 15 into func bpf_map_lookup_elem",
+		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
+	},
+	{
+		"prevent map lookup in sockhash",
+		.insns = {
+			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+			BPF_LD_MAP_FD(BPF_REG_1, 0),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_map_lookup_elem),
+			BPF_EXIT_INSN(),
+		},
+		.fixup_map6 = { 3 },
+		.result = REJECT,
+		.errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem",
+		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
+	},
+	{
+		"prevent map lookup in xskmap",
+		.insns = {
+			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+			BPF_LD_MAP_FD(BPF_REG_1, 0),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_map_lookup_elem),
+			BPF_EXIT_INSN(),
+		},
+		.fixup_map7 = { 3 },
+		.result = REJECT,
+		.errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem",
+		.prog_type = BPF_PROG_TYPE_XDP,
+	},
+	{
+		"prevent map lookup in stack trace",
+		.insns = {
+			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+			BPF_LD_MAP_FD(BPF_REG_1, 0),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_map_lookup_elem),
+			BPF_EXIT_INSN(),
+		},
+		.fixup_map8 = { 3 },
+		.result = REJECT,
+		.errstr = "cannot pass map_type 7 into func bpf_map_lookup_elem",
+		.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+	},
+	{
+		"prevent map lookup in prog array",
+		.insns = {
+			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+			BPF_LD_MAP_FD(BPF_REG_1, 0),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+				     BPF_FUNC_map_lookup_elem),
+			BPF_EXIT_INSN(),
+		},
+		.fixup_prog2 = { 3 },
+		.result = REJECT,
+		.errstr = "cannot pass map_type 3 into func bpf_map_lookup_elem",
+	},
 	{
 		"valid map access into an array with a constant",
 		.insns = {
@@ -12755,6 +12838,10 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
 	int *fixup_map2 = test->fixup_map2;
 	int *fixup_map3 = test->fixup_map3;
 	int *fixup_map4 = test->fixup_map4;
+	int *fixup_map5 = test->fixup_map5;
+	int *fixup_map6 = test->fixup_map6;
+	int *fixup_map7 = test->fixup_map7;
+	int *fixup_map8 = test->fixup_map8;
 	int *fixup_prog1 = test->fixup_prog1;
 	int *fixup_prog2 = test->fixup_prog2;
 	int *fixup_map_in_map = test->fixup_map_in_map;
@@ -12843,6 +12930,38 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
 			fixup_percpu_cgroup_storage++;
 		} while (*fixup_percpu_cgroup_storage);
 	}
+	if (*fixup_map5) {
+		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
+					sizeof(int), 1);
+		do {
+			prog[*fixup_map5].imm = map_fds[9];
+			fixup_map5++;
+		} while (*fixup_map5);
+	}
+	if (*fixup_map6) {
+		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
+					sizeof(int), 1);
+		do {
+			prog[*fixup_map6].imm = map_fds[10];
+			fixup_map6++;
+		} while (*fixup_map6);
+	}
+	if (*fixup_map7) {
+		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
+					sizeof(int), 1);
+		do {
+			prog[*fixup_map7].imm = map_fds[11];
+			fixup_map7++;
+		} while (*fixup_map7);
+	}
+	if (*fixup_map8) {
+		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
+					 sizeof(u64), 1);
+		do {
+			prog[*fixup_map8].imm = map_fds[12];
+			fixup_map8++;
+		} while (fixup_map8);
+	}
 }
 
 static void do_test_single(struct bpf_test *test, bool unpriv,
-- 
2.17.1

Re: [RFC v2 bpf-next 3/5] tools/bpf: bpftool, split the function do_dump()

[Jakub Kicinski]

On Tue,  2 Oct 2018 14:35:17 +0900, Prashant Bhole wrote:
> do_dump() function in bpftool/map.c has deep indentations. In order
> to reduce deep indent, let's move element printing code out of
> do_dump() into dump_map_elem() function.
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Re: [RFC v2 bpf-next 4/5] tools/bpf: bpftool, print strerror when map lookup error occurs

[Jakub Kicinski]

On Tue,  2 Oct 2018 14:35:18 +0900, Prashant Bhole wrote:
> Since map lookup error can be ENOENT or EOPNOTSUPP, let's print
> strerror() as error message in normal and JSON output.
> 
> This patch adds helper function print_entry_error() to print
> entry from lookup error occurs
> 
> Example: Following example dumps a map which does not support lookup.
> 
...

Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Thank you!

Re: [RFC v2 bpf-next 2/5] bpf: return EOPNOTSUPP when map lookup isn't supported

[Alexei Starovoitov]

On Tue, Oct 02, 2018 at 02:35:16PM +0900, Prashant Bhole wrote:
> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
> map types:
> - BPF_MAP_TYPE_PROG_ARRAY
> - BPF_MAP_TYPE_STACK_TRACE
> - BPF_MAP_TYPE_XSKMAP
> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  kernel/bpf/arraymap.c | 2 +-
>  kernel/bpf/sockmap.c  | 2 +-
>  kernel/bpf/stackmap.c | 2 +-
>  kernel/bpf/xskmap.c   | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index dded84cbe814..24583da9ffd1 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>  
>  static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>  {
> -	return NULL;
> +	return ERR_PTR(-EOPNOTSUPP);

last time we discussed that the verifier should make sure that
lookup is not called from bpf program for these map types.
I'd like to see test cases in test_verifier.c for these map types
to make sure we don't introduce crashes.

Re: [RFC v2 bpf-next 2/5] bpf: return EOPNOTSUPP when map lookup isn't supported

[Prashant Bhole]

On 10/5/2018 9:10 AM, Alexei Starovoitov wrote:
> On Tue, Oct 02, 2018 at 02:35:16PM +0900, Prashant Bhole wrote:
>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>> map types:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   kernel/bpf/arraymap.c | 2 +-
>>   kernel/bpf/sockmap.c  | 2 +-
>>   kernel/bpf/stackmap.c | 2 +-
>>   kernel/bpf/xskmap.c   | 2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index dded84cbe814..24583da9ffd1 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>   
>>   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>>   {
>> -	return NULL;
>> +	return ERR_PTR(-EOPNOTSUPP);
> 
> last time we discussed that the verifier should make sure that
> lookup is not called from bpf program for these map types.
> I'd like to see test cases in test_verifier.c for these map types
> to make sure we don't introduce crashes.

Hi Alexei,
Patch 05/05 adds such tests in test_verifier.c. Please review those 
changes. Thank you.

-Prashant

Re: [RFC v2 bpf-next 2/5] bpf: return EOPNOTSUPP when map lookup isn't supported

[Alexei Starovoitov]

On Fri, Oct 05, 2018 at 09:16:04AM +0900, Prashant Bhole wrote:
> 
> 
> On 10/5/2018 9:10 AM, Alexei Starovoitov wrote:
> > On Tue, Oct 02, 2018 at 02:35:16PM +0900, Prashant Bhole wrote:
> > > Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
> > > map types:
> > > - BPF_MAP_TYPE_PROG_ARRAY
> > > - BPF_MAP_TYPE_STACK_TRACE
> > > - BPF_MAP_TYPE_XSKMAP
> > > - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
> > > 
> > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > > ---
> > >   kernel/bpf/arraymap.c | 2 +-
> > >   kernel/bpf/sockmap.c  | 2 +-
> > >   kernel/bpf/stackmap.c | 2 +-
> > >   kernel/bpf/xskmap.c   | 2 +-
> > >   4 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> > > index dded84cbe814..24583da9ffd1 100644
> > > --- a/kernel/bpf/arraymap.c
> > > +++ b/kernel/bpf/arraymap.c
> > > @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
> > >   static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
> > >   {
> > > -	return NULL;
> > > +	return ERR_PTR(-EOPNOTSUPP);
> > 
> > last time we discussed that the verifier should make sure that
> > lookup is not called from bpf program for these map types.
> > I'd like to see test cases in test_verifier.c for these map types
> > to make sure we don't introduce crashes.
> 
> Hi Alexei,
> Patch 05/05 adds such tests in test_verifier.c. Please review those changes.

ahh. missed it. sorry about that. looking...

Re: [RFC v2 bpf-next 5/5] selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog

[Alexei Starovoitov]

On Tue, Oct 02, 2018 at 02:35:19PM +0900, Prashant Bhole wrote:
> map_lookup_elem isn't supported by certain map types like:
> - BPF_MAP_TYPE_PROG_ARRAY
> - BPF_MAP_TYPE_STACK_TRACE
> - BPF_MAP_TYPE_XSKMAP
> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
> Let's add verfier tests to check whether verifier prevents
> bpf_map_lookup_elem call on above programs from bpf program.
> 
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 121 +++++++++++++++++++-
>  1 file changed, 120 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c7d25f23baf9..afa7e67f66e4 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -47,7 +47,7 @@
>  
>  #define MAX_INSNS	BPF_MAXINSNS
>  #define MAX_FIXUPS	8
> -#define MAX_NR_MAPS	8
> +#define MAX_NR_MAPS	13
>  #define POINTER_VALUE	0xcafe4all
>  #define TEST_DATA_LEN	64
>  
> @@ -64,6 +64,10 @@ struct bpf_test {
>  	int fixup_map2[MAX_FIXUPS];
>  	int fixup_map3[MAX_FIXUPS];
>  	int fixup_map4[MAX_FIXUPS];
> +	int fixup_map5[MAX_FIXUPS];
> +	int fixup_map6[MAX_FIXUPS];
> +	int fixup_map7[MAX_FIXUPS];
> +	int fixup_map8[MAX_FIXUPS];
>  	int fixup_prog1[MAX_FIXUPS];
>  	int fixup_prog2[MAX_FIXUPS];
>  	int fixup_map_in_map[MAX_FIXUPS];
> @@ -4391,6 +4395,85 @@ static struct bpf_test tests[] = {
>  		.errstr = "invalid access to packet",
>  		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
>  	},
> +	{
> +		"prevent map lookup in sockmap",
> +		.insns = {
> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +				     BPF_FUNC_map_lookup_elem),
> +			BPF_EXIT_INSN(),
> +		},
> +		.fixup_map5 = { 3 },
> +		.result = REJECT,
> +		.errstr = "cannot pass map_type 15 into func bpf_map_lookup_elem",
> +		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
> +	},
> +	{
> +		"prevent map lookup in sockhash",
> +		.insns = {
> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +				     BPF_FUNC_map_lookup_elem),
> +			BPF_EXIT_INSN(),
> +		},
> +		.fixup_map6 = { 3 },
> +		.result = REJECT,
> +		.errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem",
> +		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
> +	},
> +	{
> +		"prevent map lookup in xskmap",
> +		.insns = {
> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +				     BPF_FUNC_map_lookup_elem),
> +			BPF_EXIT_INSN(),
> +		},
> +		.fixup_map7 = { 3 },
> +		.result = REJECT,
> +		.errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem",
> +		.prog_type = BPF_PROG_TYPE_XDP,
> +	},
> +	{
> +		"prevent map lookup in stack trace",
> +		.insns = {
> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +				     BPF_FUNC_map_lookup_elem),
> +			BPF_EXIT_INSN(),
> +		},
> +		.fixup_map8 = { 3 },
> +		.result = REJECT,
> +		.errstr = "cannot pass map_type 7 into func bpf_map_lookup_elem",
> +		.prog_type = BPF_PROG_TYPE_PERF_EVENT,
> +	},
> +	{
> +		"prevent map lookup in prog array",
> +		.insns = {
> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> +				     BPF_FUNC_map_lookup_elem),
> +			BPF_EXIT_INSN(),
> +		},
> +		.fixup_prog2 = { 3 },
> +		.result = REJECT,
> +		.errstr = "cannot pass map_type 3 into func bpf_map_lookup_elem",

excellent tests. exactly what I was hoping to see.

> +	},
>  	{
>  		"valid map access into an array with a constant",
>  		.insns = {
> @@ -12755,6 +12838,10 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
>  	int *fixup_map2 = test->fixup_map2;
>  	int *fixup_map3 = test->fixup_map3;
>  	int *fixup_map4 = test->fixup_map4;
> +	int *fixup_map5 = test->fixup_map5;
> +	int *fixup_map6 = test->fixup_map6;
> +	int *fixup_map7 = test->fixup_map7;
> +	int *fixup_map8 = test->fixup_map8;
>  	int *fixup_prog1 = test->fixup_prog1;
>  	int *fixup_prog2 = test->fixup_prog2;
>  	int *fixup_map_in_map = test->fixup_map_in_map;
> @@ -12843,6 +12930,38 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
>  			fixup_percpu_cgroup_storage++;
>  		} while (*fixup_percpu_cgroup_storage);
>  	}
> +	if (*fixup_map5) {
> +		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
> +					sizeof(int), 1);
> +		do {
> +			prog[*fixup_map5].imm = map_fds[9];
> +			fixup_map5++;
> +		} while (*fixup_map5);
> +	}
> +	if (*fixup_map6) {
> +		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
> +					sizeof(int), 1);
> +		do {
> +			prog[*fixup_map6].imm = map_fds[10];
> +			fixup_map6++;
> +		} while (*fixup_map6);
> +	}
> +	if (*fixup_map7) {
> +		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
> +					sizeof(int), 1);
> +		do {
> +			prog[*fixup_map7].imm = map_fds[11];
> +			fixup_map7++;
> +		} while (*fixup_map7);
> +	}
> +	if (*fixup_map8) {
> +		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
> +					 sizeof(u64), 1);
> +		do {
> +			prog[*fixup_map8].imm = map_fds[12];
> +			fixup_map8++;
> +		} while (fixup_map8);

I understand that you're following the existing naming convention
with fixup_mapN, but it was ugly before and these 4 additions
make it completely unreadable.

Could you please refactor the old names:
fixup_map1 -> fixup_map_hash_8b
fixup_map2 -> fixup_map_hash_48b (pls double check my math)
fixup_map3 -> fixup_map_hash_16b
fixup_map4 -> fixup_map_array_48b

then your new diff will use
fixup_map5 -> fixup_map_sockmap
fixup_map6 -> fixup_map_sockhash
...

and please drop rfc tag from the next respin.
Thanks!

Re: [RFC v2 bpf-next 5/5] selftests/bpf: verifier, check bpf_map_lookup_elem access in bpf prog

[Prashant Bhole]

On 10/5/2018 10:51 AM, Alexei Starovoitov wrote:
> On Tue, Oct 02, 2018 at 02:35:19PM +0900, Prashant Bhole wrote:
>> map_lookup_elem isn't supported by certain map types like:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>> Let's add verfier tests to check whether verifier prevents
>> bpf_map_lookup_elem call on above programs from bpf program.
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>>   tools/testing/selftests/bpf/test_verifier.c | 121 +++++++++++++++++++-
>>   1 file changed, 120 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>> index c7d25f23baf9..afa7e67f66e4 100644
>> --- a/tools/testing/selftests/bpf/test_verifier.c
>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>> @@ -47,7 +47,7 @@
>>   
>>   #define MAX_INSNS	BPF_MAXINSNS
>>   #define MAX_FIXUPS	8
>> -#define MAX_NR_MAPS	8
>> +#define MAX_NR_MAPS	13
>>   #define POINTER_VALUE	0xcafe4all
>>   #define TEST_DATA_LEN	64
>>   
>> @@ -64,6 +64,10 @@ struct bpf_test {
>>   	int fixup_map2[MAX_FIXUPS];
>>   	int fixup_map3[MAX_FIXUPS];
>>   	int fixup_map4[MAX_FIXUPS];
>> +	int fixup_map5[MAX_FIXUPS];
>> +	int fixup_map6[MAX_FIXUPS];
>> +	int fixup_map7[MAX_FIXUPS];
>> +	int fixup_map8[MAX_FIXUPS];
>>   	int fixup_prog1[MAX_FIXUPS];
>>   	int fixup_prog2[MAX_FIXUPS];
>>   	int fixup_map_in_map[MAX_FIXUPS];
>> @@ -4391,6 +4395,85 @@ static struct bpf_test tests[] = {
>>   		.errstr = "invalid access to packet",
>>   		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
>>   	},
>> +	{
>> +		"prevent map lookup in sockmap",
>> +		.insns = {
>> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
>> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>> +				     BPF_FUNC_map_lookup_elem),
>> +			BPF_EXIT_INSN(),
>> +		},
>> +		.fixup_map5 = { 3 },
>> +		.result = REJECT,
>> +		.errstr = "cannot pass map_type 15 into func bpf_map_lookup_elem",
>> +		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
>> +	},
>> +	{
>> +		"prevent map lookup in sockhash",
>> +		.insns = {
>> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
>> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>> +				     BPF_FUNC_map_lookup_elem),
>> +			BPF_EXIT_INSN(),
>> +		},
>> +		.fixup_map6 = { 3 },
>> +		.result = REJECT,
>> +		.errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem",
>> +		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
>> +	},
>> +	{
>> +		"prevent map lookup in xskmap",
>> +		.insns = {
>> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
>> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>> +				     BPF_FUNC_map_lookup_elem),
>> +			BPF_EXIT_INSN(),
>> +		},
>> +		.fixup_map7 = { 3 },
>> +		.result = REJECT,
>> +		.errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem",
>> +		.prog_type = BPF_PROG_TYPE_XDP,
>> +	},
>> +	{
>> +		"prevent map lookup in stack trace",
>> +		.insns = {
>> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
>> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>> +				     BPF_FUNC_map_lookup_elem),
>> +			BPF_EXIT_INSN(),
>> +		},
>> +		.fixup_map8 = { 3 },
>> +		.result = REJECT,
>> +		.errstr = "cannot pass map_type 7 into func bpf_map_lookup_elem",
>> +		.prog_type = BPF_PROG_TYPE_PERF_EVENT,
>> +	},
>> +	{
>> +		"prevent map lookup in prog array",
>> +		.insns = {
>> +			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> +			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> +			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> +			BPF_LD_MAP_FD(BPF_REG_1, 0),
>> +			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
>> +				     BPF_FUNC_map_lookup_elem),
>> +			BPF_EXIT_INSN(),
>> +		},
>> +		.fixup_prog2 = { 3 },
>> +		.result = REJECT,
>> +		.errstr = "cannot pass map_type 3 into func bpf_map_lookup_elem",
> 
> excellent tests. exactly what I was hoping to see.
> 
>> +	},
>>   	{
>>   		"valid map access into an array with a constant",
>>   		.insns = {
>> @@ -12755,6 +12838,10 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
>>   	int *fixup_map2 = test->fixup_map2;
>>   	int *fixup_map3 = test->fixup_map3;
>>   	int *fixup_map4 = test->fixup_map4;
>> +	int *fixup_map5 = test->fixup_map5;
>> +	int *fixup_map6 = test->fixup_map6;
>> +	int *fixup_map7 = test->fixup_map7;
>> +	int *fixup_map8 = test->fixup_map8;
>>   	int *fixup_prog1 = test->fixup_prog1;
>>   	int *fixup_prog2 = test->fixup_prog2;
>>   	int *fixup_map_in_map = test->fixup_map_in_map;
>> @@ -12843,6 +12930,38 @@ static void do_test_fixup(struct bpf_test *test, struct bpf_insn *prog,
>>   			fixup_percpu_cgroup_storage++;
>>   		} while (*fixup_percpu_cgroup_storage);
>>   	}
>> +	if (*fixup_map5) {
>> +		map_fds[9] = create_map(BPF_MAP_TYPE_SOCKMAP, sizeof(int),
>> +					sizeof(int), 1);
>> +		do {
>> +			prog[*fixup_map5].imm = map_fds[9];
>> +			fixup_map5++;
>> +		} while (*fixup_map5);
>> +	}
>> +	if (*fixup_map6) {
>> +		map_fds[10] = create_map(BPF_MAP_TYPE_SOCKHASH, sizeof(int),
>> +					sizeof(int), 1);
>> +		do {
>> +			prog[*fixup_map6].imm = map_fds[10];
>> +			fixup_map6++;
>> +		} while (*fixup_map6);
>> +	}
>> +	if (*fixup_map7) {
>> +		map_fds[11] = create_map(BPF_MAP_TYPE_XSKMAP, sizeof(int),
>> +					sizeof(int), 1);
>> +		do {
>> +			prog[*fixup_map7].imm = map_fds[11];
>> +			fixup_map7++;
>> +		} while (*fixup_map7);
>> +	}
>> +	if (*fixup_map8) {
>> +		map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
>> +					 sizeof(u64), 1);
>> +		do {
>> +			prog[*fixup_map8].imm = map_fds[12];
>> +			fixup_map8++;
>> +		} while (fixup_map8);
> 
> I understand that you're following the existing naming convention
> with fixup_mapN, but it was ugly before and these 4 additions
> make it completely unreadable.
> 
> Could you please refactor the old names:
> fixup_map1 -> fixup_map_hash_8b
> fixup_map2 -> fixup_map_hash_48b (pls double check my math)
> fixup_map3 -> fixup_map_hash_16b
> fixup_map4 -> fixup_map_array_48b
> 
> then your new diff will use
> fixup_map5 -> fixup_map_sockmap
> fixup_map6 -> fixup_map_sockhash
> ...
> 
> and please drop rfc tag from the next respin.
> Thanks!
> 

Thanks for reviewing. I will fix the naming convention in the next patch 
series.

-Prashant