[PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Martin KaFai Lau]

This series allows the outer map to be updated with inner map in different
size as long as it is safe (meaning the max_entries is not used in the
verification time during prog load).

Please see individual patch for details.

Martin KaFai Lau (3):
  bpf: Clean up inner map type check
  bpf: Relax the max_entries check for inner map
  bpf: selftests: Add test for different inner map size

 include/linux/bpf.h                           | 18 +++++-
 include/linux/bpf_types.h                     | 64 +++++++++++--------
 kernel/bpf/btf.c                              |  2 +-
 kernel/bpf/map_in_map.c                       | 12 ++--
 kernel/bpf/syscall.c                          | 19 +++++-
 kernel/bpf/verifier.c                         |  2 +-
 .../selftests/bpf/prog_tests/btf_map_in_map.c | 12 ++++
 .../selftests/bpf/progs/test_btf_map_in_map.c | 31 +++++++++
 8 files changed, 119 insertions(+), 41 deletions(-)

-- 
2.24.1

[PATCH bpf-next 1/3] bpf: Clean up inner map type check

[Martin KaFai Lau]

This patch adds a "capability" member to "struct bpf_map".  Each map-type's
capability is tagged in the "BPF_MAP_TYPE" in bpf_types.h.  This will
clean up the individual check in bpf_map_meta_alloc() which decides
if a map can be used as an inner map.

It will be less error prone to decide its capability at the same place
as the new map type is added in bpf_types.h.  That will help to avoid
mistake like missing modification in other source files like
the map_in_map.c here.

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/linux/bpf.h       |  6 +++-
 include/linux/bpf_types.h | 62 ++++++++++++++++++++++-----------------
 kernel/bpf/btf.c          |  2 +-
 kernel/bpf/map_in_map.c   |  9 ++----
 kernel/bpf/syscall.c      | 19 ++++++++++--
 kernel/bpf/verifier.c     |  2 +-
 6 files changed, 60 insertions(+), 40 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index efe8836b5c48..1e20b9911d48 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -97,6 +97,9 @@ struct bpf_map_memory {
 	struct user_struct *user;
 };
 
+/* Cannot be used as an inner map */
+#define BPF_MAP_CAP_NO_INNER_MAP (1 << 0)
+
 struct bpf_map {
 	/* The first two cachelines with read-mostly members of which some
 	 * are also accessed in fast-path (e.g. ops, max_entries).
@@ -120,6 +123,7 @@ struct bpf_map {
 	struct bpf_map_memory memory;
 	char name[BPF_OBJ_NAME_LEN];
 	u32 btf_vmlinux_value_type_id;
+	u32 capability;
 	bool bypass_spec_v1;
 	bool frozen; /* write-once; write-protected by freeze_mutex */
 	/* 22 bytes hole */
@@ -1037,7 +1041,7 @@ extern const struct file_operations bpf_iter_fops;
 #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
 	extern const struct bpf_prog_ops _name ## _prog_ops; \
 	extern const struct bpf_verifier_ops _name ## _verifier_ops;
-#define BPF_MAP_TYPE(_id, _ops) \
+#define BPF_MAP_TYPE(_id, _ops, map_cap) \
 	extern const struct bpf_map_ops _ops;
 #define BPF_LINK_TYPE(_id, _name)
 #include <linux/bpf_types.h>
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 29d22752fc87..652f17d646dd 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -76,47 +76,55 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
 #endif /* CONFIG_BPF_LSM */
 #endif
 
-BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_PROG_ARRAY, prog_array_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_PERF_EVENT_ARRAY, perf_event_array_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops, 0)
+/* prog_array->aux->{type,jited} is a runtime binding.
+ * Doing static check alone in the verifier is not enough,
+ * so BPF_MAP_CAP_NO_INNTER_MAP is needed.
+ */
+BPF_MAP_TYPE(BPF_MAP_TYPE_PROG_ARRAY, prog_array_map_ops,
+	     BPF_MAP_CAP_NO_INNER_MAP)
+BPF_MAP_TYPE(BPF_MAP_TYPE_PERF_EVENT_ARRAY, perf_event_array_map_ops, 0)
 #ifdef CONFIG_CGROUPS
-BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops, 0)
 #endif
 #ifdef CONFIG_CGROUP_BPF
-BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
-#endif
-BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_LRU_HASH, htab_lru_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_LRU_PERCPU_HASH, htab_lru_percpu_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_LPM_TRIE, trie_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops,
+	     BPF_MAP_CAP_NO_INNER_MAP)
+BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops,
+	     BPF_MAP_CAP_NO_INNER_MAP)
+#endif
+BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_LRU_HASH, htab_lru_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_LRU_PERCPU_HASH, htab_lru_percpu_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_LPM_TRIE, trie_map_ops, 0)
 #ifdef CONFIG_PERF_EVENTS
-BPF_MAP_TYPE(BPF_MAP_TYPE_STACK_TRACE, stack_trace_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_STACK_TRACE, stack_trace_map_ops, 0)
 #endif
-BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY_OF_MAPS, array_of_maps_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS, htab_of_maps_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY_OF_MAPS, array_of_maps_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS, htab_of_maps_map_ops, 0)
 #ifdef CONFIG_NET
-BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops, 0)
 #if defined(CONFIG_BPF_STREAM_PARSER)
-BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops, 0)
 #endif
-BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops, 0)
 #if defined(CONFIG_XDP_SOCKETS)
-BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops, 0)
 #endif
 #ifdef CONFIG_INET
-BPF_MAP_TYPE(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY, reuseport_array_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY, reuseport_array_ops, 0)
 #endif
 #endif
-BPF_MAP_TYPE(BPF_MAP_TYPE_QUEUE, queue_map_ops)
-BPF_MAP_TYPE(BPF_MAP_TYPE_STACK, stack_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_QUEUE, queue_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_STACK, stack_map_ops, 0)
 #if defined(CONFIG_BPF_JIT)
-BPF_MAP_TYPE(BPF_MAP_TYPE_STRUCT_OPS, bpf_struct_ops_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_STRUCT_OPS, bpf_struct_ops_map_ops,
+	     BPF_MAP_CAP_NO_INNER_MAP)
 #endif
 
 BPF_LINK_TYPE(BPF_LINK_TYPE_RAW_TRACEPOINT, raw_tracepoint)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 58c9af1d4808..6b74ab8f8530 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3481,7 +3481,7 @@ extern char __weak __start_BTF[];
 extern char __weak __stop_BTF[];
 extern struct btf *btf_vmlinux;
 
-#define BPF_MAP_TYPE(_id, _ops)
+#define BPF_MAP_TYPE(_id, _ops, map_cap)
 #define BPF_LINK_TYPE(_id, _name)
 static union {
 	struct bpf_ctx_convert {
diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
index 17738c93bec8..6e1286ad7b76 100644
--- a/kernel/bpf/map_in_map.c
+++ b/kernel/bpf/map_in_map.c
@@ -17,13 +17,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 	if (IS_ERR(inner_map))
 		return inner_map;
 
-	/* prog_array->aux->{type,jited} is a runtime binding.
-	 * Doing static check alone in the verifier is not enough.
-	 */
-	if (inner_map->map_type == BPF_MAP_TYPE_PROG_ARRAY ||
-	    inner_map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
-	    inner_map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE ||
-	    inner_map->map_type == BPF_MAP_TYPE_STRUCT_OPS) {
+	if (inner_map->capability & BPF_MAP_CAP_NO_INNER_MAP) {
 		fdput(f);
 		return ERR_PTR(-ENOTSUPP);
 	}
@@ -56,6 +50,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 	inner_map_meta->map_flags = inner_map->map_flags;
 	inner_map_meta->max_entries = inner_map->max_entries;
 	inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
+	inner_map_meta->capability = inner_map->capability;
 
 	/* Misc members not needed in bpf_map_meta_equal() check. */
 	inner_map_meta->ops = inner_map->ops;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 431241c74614..f93a96b8d440 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -49,7 +49,7 @@ int sysctl_unprivileged_bpf_disabled __read_mostly;
 
 static const struct bpf_map_ops * const bpf_map_types[] = {
 #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type)
-#define BPF_MAP_TYPE(_id, _ops) \
+#define BPF_MAP_TYPE(_id, _ops, map_cap) \
 	[_id] = &_ops,
 #define BPF_LINK_TYPE(_id, _name)
 #include <linux/bpf_types.h>
@@ -58,6 +58,17 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
 #undef BPF_LINK_TYPE
 };
 
+static const u32 bpf_map_caps[] = {
+#define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type)
+#define BPF_MAP_TYPE(_id, _ops, map_cap) \
+	[_id] = map_cap,
+#define BPF_LINK_TYPE(_id, _name)
+#include <linux/bpf_types.h>
+#undef BPF_PROG_TYPE
+#undef BPF_MAP_TYPE
+#undef BPF_LINK_TYPE
+};
+
 /*
  * If we're handed a bigger struct than we know of, ensure all the unknown bits
  * are 0 - i.e. new user-space does not rely on any kernel feature extensions
@@ -131,6 +142,8 @@ static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
 		return map;
 	map->ops = ops;
 	map->map_type = type;
+	map->capability = bpf_map_caps[type];
+
 	return map;
 }
 
@@ -1551,7 +1564,7 @@ static int map_freeze(const union bpf_attr *attr)
 static const struct bpf_prog_ops * const bpf_prog_types[] = {
 #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
 	[_id] = & _name ## _prog_ops,
-#define BPF_MAP_TYPE(_id, _ops)
+#define BPF_MAP_TYPE(_id, _ops, map_cap)
 #define BPF_LINK_TYPE(_id, _name)
 #include <linux/bpf_types.h>
 #undef BPF_PROG_TYPE
@@ -2333,7 +2346,7 @@ static int bpf_link_release(struct inode *inode, struct file *filp)
 
 #ifdef CONFIG_PROC_FS
 #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type)
-#define BPF_MAP_TYPE(_id, _ops)
+#define BPF_MAP_TYPE(_id, _ops, map_cap)
 #define BPF_LINK_TYPE(_id, _name) [_id] = #_name,
 static const char *bpf_link_type_strs[] = {
 	[BPF_LINK_TYPE_UNSPEC] = "<invalid>",
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2ed8351f47a4..5f3b97d25c4e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -27,7 +27,7 @@
 static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
 #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
 	[_id] = & _name ## _verifier_ops,
-#define BPF_MAP_TYPE(_id, _ops)
+#define BPF_MAP_TYPE(_id, _ops, map_cap)
 #define BPF_LINK_TYPE(_id, _name)
 #include <linux/bpf_types.h>
 #undef BPF_PROG_TYPE
-- 
2.24.1

[PATCH bpf-next 2/3] bpf: Relax the max_entries check for inner map

[Martin KaFai Lau]

This patch relaxes the max_entries check for most of the inner map types
during an update to the outer map.  The max_entries of those map types
are only used in runtime.  By doing this, an inner map with different
size can be updated to the outer map in runtime.

The max_entries of arraymap and xskmap are used statically
in verification time to generate the inline code, so they
are excluded in this patch.

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/linux/bpf.h       | 12 ++++++++++++
 include/linux/bpf_types.h |  6 ++++--
 kernel/bpf/map_in_map.c   |  3 ++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1e20b9911d48..1488d2aa41f2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -99,6 +99,18 @@ struct bpf_map_memory {
 
 /* Cannot be used as an inner map */
 #define BPF_MAP_CAP_NO_INNER_MAP (1 << 0)
+/* When a prog has used map-in-map, the verifier requires
+ * an inner-map as a template to verify the access operations
+ * on the outer and inner map.  For some inner map-types,
+ * the verifier uses the inner_map's max_entries statically
+ * (e.g. to generate inline code).  If this verification
+ * time usage on max_entries applies to an inner map-type,
+ * during runtime, only the inner map with the same
+ * max_entries can be updated to this outer map.
+ *
+ * Please see bpf_map_meta_equal() for details.
+ */
+#define BPF_MAP_CAP_NO_DYNAMIC_INNER_MAP_SIZE (1 << 1)
 
 struct bpf_map {
 	/* The first two cachelines with read-mostly members of which some
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 652f17d646dd..4b350f9ad486 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -76,7 +76,8 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
 #endif /* CONFIG_BPF_LSM */
 #endif
 
-BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops,
+	     BPF_MAP_CAP_NO_DYNAMIC_INNER_MAP_SIZE)
 BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops, 0)
 /* prog_array->aux->{type,jited} is a runtime binding.
  * Doing static check alone in the verifier is not enough,
@@ -114,7 +115,8 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops, 0)
 #endif
 BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops, 0)
 #if defined(CONFIG_XDP_SOCKETS)
-BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops, 0)
+BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops,
+	     BPF_MAP_CAP_NO_DYNAMIC_INNER_MAP_SIZE)
 #endif
 #ifdef CONFIG_INET
 BPF_MAP_TYPE(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY, reuseport_array_ops, 0)
diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
index 6e1286ad7b76..bee1fcfd64f2 100644
--- a/kernel/bpf/map_in_map.c
+++ b/kernel/bpf/map_in_map.c
@@ -77,7 +77,8 @@ bool bpf_map_meta_equal(const struct bpf_map *meta0,
 		meta0->key_size == meta1->key_size &&
 		meta0->value_size == meta1->value_size &&
 		meta0->map_flags == meta1->map_flags &&
-		meta0->max_entries == meta1->max_entries;
+		(meta0->max_entries == meta1->max_entries ||
+		 !(meta0->capability & BPF_MAP_CAP_NO_DYNAMIC_INNER_MAP_SIZE));
 }
 
 void *bpf_map_fd_get_ptr(struct bpf_map *map,
-- 
2.24.1

[PATCH bpf-next 3/3] bpf: selftests: Add test for different inner map size

[Martin KaFai Lau]

This patch tests the inner map size can be different
for reuseport_sockarray but has to be the same for
arraymap.

Cc: Andrey Ignatov <rdna@fb.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 .../selftests/bpf/prog_tests/btf_map_in_map.c | 12 +++++++
 .../selftests/bpf/progs/test_btf_map_in_map.c | 31 +++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
index f7ee8fa377ad..7ae767d15608 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
@@ -44,6 +44,18 @@ void test_btf_map_in_map(void)
 	bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map2), &key, &val);
 	CHECK(val != 3, "inner2", "got %d != exp %d\n", val, 3);
 
+	val = bpf_map__fd(skel->maps.sockarr_sz2);
+	err = bpf_map_update_elem(bpf_map__fd(skel->maps.outer_sockarr_sz1),
+				  &key, &val, 0);
+	CHECK(err, "outer_sockarr inner map size check",
+	      "cannot use an inner_map with different size\n");
+
+	val = bpf_map__fd(skel->maps.inner_map_sz2);
+	err = bpf_map_update_elem(bpf_map__fd(skel->maps.outer_arr), &key,
+				  &val, 0);
+	CHECK(!err, "outer_arr inner map size check",
+	      "incorrectly updated with an inner_map in different size\n");
+
 cleanup:
 	test_btf_map_in_map__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_btf_map_in_map.c b/tools/testing/selftests/bpf/progs/test_btf_map_in_map.c
index e5093796be97..0b8e04a817f6 100644
--- a/tools/testing/selftests/bpf/progs/test_btf_map_in_map.c
+++ b/tools/testing/selftests/bpf/progs/test_btf_map_in_map.c
@@ -11,6 +11,13 @@ struct inner_map {
 } inner_map1 SEC(".maps"),
   inner_map2 SEC(".maps");
 
+struct inner_map_sz2 {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 2);
+	__type(key, int);
+	__type(value, int);
+} inner_map_sz2 SEC(".maps");
+
 struct outer_arr {
 	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
 	__uint(max_entries, 3);
@@ -50,6 +57,30 @@ struct outer_hash {
 	},
 };
 
+struct sockarr_sz1 {
+	__uint(type, BPF_MAP_TYPE_REUSEPORT_SOCKARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, int);
+} sockarr_sz1 SEC(".maps");
+
+struct sockarr_sz2 {
+	__uint(type, BPF_MAP_TYPE_REUSEPORT_SOCKARRAY);
+	__uint(max_entries, 2);
+	__type(key, int);
+	__type(value, int);
+} sockarr_sz2 SEC(".maps");
+
+struct outer_sockarr_sz1 {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(int));
+	__array(values, struct sockarr_sz1);
+} outer_sockarr_sz1 SEC(".maps") = {
+	.values = { (void *)&sockarr_sz1 },
+};
+
 int input = 0;
 
 SEC("raw_tp/sys_enter")
-- 
2.24.1

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Andrii Nakryiko]

On Thu, May 21, 2020 at 12:18 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> This series allows the outer map to be updated with inner map in different
> size as long as it is safe (meaning the max_entries is not used in the
> verification time during prog load).
>
> Please see individual patch for details.
>

Few thoughts:

1. You describe WHAT, but not necessarily WHY. Can you please
elaborate in descriptions what motivates these changes?
2. IMO, "capabilities" is word that way too strongly correlates with
Linux capabilities framework, it's just confusing. It's also more of a
property of a map type, than what map is capable of, but it's more
philosophical distinction, of course :)
3. I'm honestly not convinced that patch #1 qualifies as a clean up. I
think one specific check for types of maps that are not compatible
with map-in-map is just fine. Instead you are spreading this bit flags
into a long list of maps, most of which ARE compatible. It's just hard
to even see which ones are not compatible. I like current way better.
4. Then for size check change, again, it's really much simpler and
cleaner just to have a special case in check in bpf_map_meta_equal for
cases where map size matters.
5. I also wonder if for those inner maps for which size doesn't
matter, maybe we should set max_elements to zero when setting
inner_meta to show that size doesn't matter? This is minor, though.


> Martin KaFai Lau (3):
>   bpf: Clean up inner map type check
>   bpf: Relax the max_entries check for inner map
>   bpf: selftests: Add test for different inner map size
>
>  include/linux/bpf.h                           | 18 +++++-
>  include/linux/bpf_types.h                     | 64 +++++++++++--------
>  kernel/bpf/btf.c                              |  2 +-
>  kernel/bpf/map_in_map.c                       | 12 ++--
>  kernel/bpf/syscall.c                          | 19 +++++-
>  kernel/bpf/verifier.c                         |  2 +-
>  .../selftests/bpf/prog_tests/btf_map_in_map.c | 12 ++++
>  .../selftests/bpf/progs/test_btf_map_in_map.c | 31 +++++++++
>  8 files changed, 119 insertions(+), 41 deletions(-)
>
> --
> 2.24.1
>

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Martin KaFai Lau]

On Thu, May 21, 2020 at 03:39:10PM -0700, Andrii Nakryiko wrote:
> On Thu, May 21, 2020 at 12:18 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > This series allows the outer map to be updated with inner map in different
> > size as long as it is safe (meaning the max_entries is not used in the
> > verification time during prog load).
> >
> > Please see individual patch for details.
> >
> 
> Few thoughts:
> 
> 1. You describe WHAT, but not necessarily WHY. Can you please
> elaborate in descriptions what motivates these changes?
There are cases where people want to update a bigger size
inner map.  I will update the cover letter.

> 2. IMO, "capabilities" is word that way too strongly correlates with
> Linux capabilities framework, it's just confusing. It's also more of a
> property of a map type, than what map is capable of, but it's more
> philosophical distinction, of course :)
Sure. I can rename it to "property"

> 3. I'm honestly not convinced that patch #1 qualifies as a clean up. I
> think one specific check for types of maps that are not compatible
> with map-in-map is just fine. Instead you are spreading this bit flags
> into a long list of maps, most of which ARE compatible.
but in one place and at the same time a new map type is added to
bpf_types.h

> It's just hard
> to even see which ones are not compatible. I like current way better.
There are multiple cases that people forgot to exclude a new map
type from map-in-map in the first attempt and fix it up later.

During the map-in-map implementation, this same concern was raised also
about how to better exclude future map type from map-in-map since
not all people has used map-in-map and it is easy to forget during
review.  Having it in one place in bpf_types.h will make this
more obvious in my opinion.  Patch 1 is an attempt to address
this earlier concern in the map-in-map implementation.

> 4. Then for size check change, again, it's really much simpler and
> cleaner just to have a special case in check in bpf_map_meta_equal for
> cases where map size matters.
It may be simpler but not necessary less fragile for future map type.

I am OK for removing patch 1 and just check for a specific
type in patch 2 but I think it is fragile for future map
type IMO.

> 5. I also wonder if for those inner maps for which size doesn't
> matter, maybe we should set max_elements to zero when setting
> inner_meta to show that size doesn't matter? This is minor, though.
> 
> 
> > Martin KaFai Lau (3):
> >   bpf: Clean up inner map type check
> >   bpf: Relax the max_entries check for inner map
> >   bpf: selftests: Add test for different inner map size
> >
> >  include/linux/bpf.h                           | 18 +++++-
> >  include/linux/bpf_types.h                     | 64 +++++++++++--------
> >  kernel/bpf/btf.c                              |  2 +-
> >  kernel/bpf/map_in_map.c                       | 12 ++--
> >  kernel/bpf/syscall.c                          | 19 +++++-
> >  kernel/bpf/verifier.c                         |  2 +-
> >  .../selftests/bpf/prog_tests/btf_map_in_map.c | 12 ++++
> >  .../selftests/bpf/progs/test_btf_map_in_map.c | 31 +++++++++
> >  8 files changed, 119 insertions(+), 41 deletions(-)
> >
> > --
> > 2.24.1
> >

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Andrii Nakryiko]

On Thu, May 21, 2020 at 3:59 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Thu, May 21, 2020 at 03:39:10PM -0700, Andrii Nakryiko wrote:
> > On Thu, May 21, 2020 at 12:18 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > This series allows the outer map to be updated with inner map in different
> > > size as long as it is safe (meaning the max_entries is not used in the
> > > verification time during prog load).
> > >
> > > Please see individual patch for details.
> > >
> >
> > Few thoughts:
> >
> > 1. You describe WHAT, but not necessarily WHY. Can you please
> > elaborate in descriptions what motivates these changes?
> There are cases where people want to update a bigger size
> inner map.  I will update the cover letter.
>
> > 2. IMO, "capabilities" is word that way too strongly correlates with
> > Linux capabilities framework, it's just confusing. It's also more of a
> > property of a map type, than what map is capable of, but it's more
> > philosophical distinction, of course :)
> Sure. I can rename it to "property"
>
> > 3. I'm honestly not convinced that patch #1 qualifies as a clean up. I
> > think one specific check for types of maps that are not compatible
> > with map-in-map is just fine. Instead you are spreading this bit flags
> > into a long list of maps, most of which ARE compatible.
> but in one place and at the same time a new map type is added to
> bpf_types.h
>
> > It's just hard
> > to even see which ones are not compatible. I like current way better.
> There are multiple cases that people forgot to exclude a new map
> type from map-in-map in the first attempt and fix it up later.
>
> During the map-in-map implementation, this same concern was raised also
> about how to better exclude future map type from map-in-map since
> not all people has used map-in-map and it is easy to forget during
> review.  Having it in one place in bpf_types.h will make this
> more obvious in my opinion.  Patch 1 is an attempt to address
> this earlier concern in the map-in-map implementation.

Ok, just invert the condition and list only types that **are** allowed
inside map-in-map. If someone forgot to add it to map-in-map check, it
can be done later when someone needs it. The point is that we have a
check and a list in one place, close to where it matters, instead of
tracing where the value of ->capabilities comes from. Finding that in
bpf_types.h is not easy and not obvious, unfortunately, and is very
distant from where it's actually checked.


>
> > 4. Then for size check change, again, it's really much simpler and
> > cleaner just to have a special case in check in bpf_map_meta_equal for
> > cases where map size matters.
> It may be simpler but not necessary less fragile for future map type.
>
> I am OK for removing patch 1 and just check for a specific
> type in patch 2 but I think it is fragile for future map
> type IMO.

Well, if we think that the good default needs to be to check size,
then similar to above, explicitly list stuff that *does not* follow
the default, i.e., maps that don't want max_elements verification. My
point still stands.

>
> > 5. I also wonder if for those inner maps for which size doesn't
> > matter, maybe we should set max_elements to zero when setting
> > inner_meta to show that size doesn't matter? This is minor, though.
> >
> >
> > > Martin KaFai Lau (3):
> > >   bpf: Clean up inner map type check
> > >   bpf: Relax the max_entries check for inner map
> > >   bpf: selftests: Add test for different inner map size
> > >
> > >  include/linux/bpf.h                           | 18 +++++-
> > >  include/linux/bpf_types.h                     | 64 +++++++++++--------
> > >  kernel/bpf/btf.c                              |  2 +-
> > >  kernel/bpf/map_in_map.c                       | 12 ++--
> > >  kernel/bpf/syscall.c                          | 19 +++++-
> > >  kernel/bpf/verifier.c                         |  2 +-
> > >  .../selftests/bpf/prog_tests/btf_map_in_map.c | 12 ++++
> > >  .../selftests/bpf/progs/test_btf_map_in_map.c | 31 +++++++++
> > >  8 files changed, 119 insertions(+), 41 deletions(-)
> > >
> > > --
> > > 2.24.1
> > >

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Alexei Starovoitov]

On Thu, May 21, 2020 at 04:10:36PM -0700, Andrii Nakryiko wrote:
> > > 4. Then for size check change, again, it's really much simpler and
> > > cleaner just to have a special case in check in bpf_map_meta_equal for
> > > cases where map size matters.
> > It may be simpler but not necessary less fragile for future map type.
> >
> > I am OK for removing patch 1 and just check for a specific
> > type in patch 2 but I think it is fragile for future map
> > type IMO.
> 
> Well, if we think that the good default needs to be to check size,
> then similar to above, explicitly list stuff that *does not* follow
> the default, i.e., maps that don't want max_elements verification. My
> point still stands.

I think consoldating map properties in bpf_types.h is much cleaner
and less error prone.
I'd only like to tweak the macro in patch 1 to avoid explicit ", 0)".
Can BPF_MAP_TYPE() macro stay as-is and additional macro introduced
for maps with properties ? BPF_MAP_TYPE_FL() ?
Or do some macro magic that the same macro can be used with 2 and 3 args?

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Martin KaFai Lau]

On Thu, May 21, 2020 at 04:16:18PM -0700, Alexei Starovoitov wrote:
> On Thu, May 21, 2020 at 04:10:36PM -0700, Andrii Nakryiko wrote:
> > > > 4. Then for size check change, again, it's really much simpler and
> > > > cleaner just to have a special case in check in bpf_map_meta_equal for
> > > > cases where map size matters.
> > > It may be simpler but not necessary less fragile for future map type.
> > >
> > > I am OK for removing patch 1 and just check for a specific
> > > type in patch 2 but I think it is fragile for future map
> > > type IMO.
> > 
> > Well, if we think that the good default needs to be to check size,
> > then similar to above, explicitly list stuff that *does not* follow
> > the default, i.e., maps that don't want max_elements verification. My
> > point still stands.
> 
> I think consoldating map properties in bpf_types.h is much cleaner
> and less error prone.
> I'd only like to tweak the macro in patch 1 to avoid explicit ", 0)".
> Can BPF_MAP_TYPE() macro stay as-is and additional macro introduced
> for maps with properties ? BPF_MAP_TYPE_FL() ?
> Or do some macro magic that the same macro can be used with 2 and 3 args?
I will give it a try to minimize the code churn.

Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries

[Andrii Nakryiko]

On Thu, May 21, 2020 at 4:16 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 21, 2020 at 04:10:36PM -0700, Andrii Nakryiko wrote:
> > > > 4. Then for size check change, again, it's really much simpler and
> > > > cleaner just to have a special case in check in bpf_map_meta_equal for
> > > > cases where map size matters.
> > > It may be simpler but not necessary less fragile for future map type.
> > >
> > > I am OK for removing patch 1 and just check for a specific
> > > type in patch 2 but I think it is fragile for future map
> > > type IMO.
> >
> > Well, if we think that the good default needs to be to check size,
> > then similar to above, explicitly list stuff that *does not* follow
> > the default, i.e., maps that don't want max_elements verification. My
> > point still stands.
>
> I think consoldating map properties in bpf_types.h is much cleaner
> and less error prone.

Consolidation is good, but then we hopefully do it for all aspects of
maps that currently have ad-hoc checks spread across a lot of places.
Just looking at map_lookup_elem in syscall.c makes me wanna cry, for
example :) I'll reply on another thread where Daniel proposed putting
everything into ops, I like that better.

> I'd only like to tweak the macro in patch 1 to avoid explicit ", 0)".
> Can BPF_MAP_TYPE() macro stay as-is and additional macro introduced
> for maps with properties ? BPF_MAP_TYPE_FL() ?
> Or do some macro magic that the same macro can be used with 2 and 3 args?