* [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries
@ 2020-05-21 19:17 Martin KaFai Lau
2020-05-21 19:17 ` [PATCH bpf-next 1/3] bpf: Clean up inner map type check Martin KaFai Lau
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Martin KaFai Lau @ 2020-05-21 19:17 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH bpf-next 1/3] bpf: Clean up inner map type check 2020-05-21 19:17 [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau @ 2020-05-21 19:17 ` Martin KaFai Lau 2020-05-21 19:18 ` [PATCH bpf-next 2/3] bpf: Relax the max_entries check for inner map Martin KaFai Lau ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Martin KaFai Lau @ 2020-05-21 19:17 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev, Andrey Ignatov 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next 2/3] bpf: Relax the max_entries check for inner map 2020-05-21 19:17 [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau 2020-05-21 19:17 ` [PATCH bpf-next 1/3] bpf: Clean up inner map type check Martin KaFai Lau @ 2020-05-21 19:18 ` Martin KaFai Lau 2020-05-21 19:18 ` [PATCH bpf-next 3/3] bpf: selftests: Add test for different inner map size Martin KaFai Lau 2020-05-21 22:39 ` [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Andrii Nakryiko 3 siblings, 0 replies; 10+ messages in thread From: Martin KaFai Lau @ 2020-05-21 19:18 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev, Andrey Ignatov 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next 3/3] bpf: selftests: Add test for different inner map size 2020-05-21 19:17 [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau 2020-05-21 19:17 ` [PATCH bpf-next 1/3] bpf: Clean up inner map type check Martin KaFai Lau 2020-05-21 19:18 ` [PATCH bpf-next 2/3] bpf: Relax the max_entries check for inner map Martin KaFai Lau @ 2020-05-21 19:18 ` Martin KaFai Lau 2020-05-21 22:39 ` [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Andrii Nakryiko 3 siblings, 0 replies; 10+ messages in thread From: Martin KaFai Lau @ 2020-05-21 19:18 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev, Andrey Ignatov 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 19:17 [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau ` (2 preceding siblings ...) 2020-05-21 19:18 ` [PATCH bpf-next 3/3] bpf: selftests: Add test for different inner map size Martin KaFai Lau @ 2020-05-21 22:39 ` Andrii Nakryiko 2020-05-21 22:59 ` Martin KaFai Lau 3 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-05-21 22:39 UTC (permalink / raw) To: Martin KaFai Lau Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 22:39 ` [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Andrii Nakryiko @ 2020-05-21 22:59 ` Martin KaFai Lau 2020-05-21 23:10 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Martin KaFai Lau @ 2020-05-21 22:59 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 22:59 ` Martin KaFai Lau @ 2020-05-21 23:10 ` Andrii Nakryiko 2020-05-21 23:16 ` Alexei Starovoitov 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-05-21 23:10 UTC (permalink / raw) To: Martin KaFai Lau Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 23:10 ` Andrii Nakryiko @ 2020-05-21 23:16 ` Alexei Starovoitov 2020-05-22 0:06 ` Martin KaFai Lau 2020-05-26 17:47 ` Andrii Nakryiko 0 siblings, 2 replies; 10+ messages in thread From: Alexei Starovoitov @ 2020-05-21 23:16 UTC (permalink / raw) To: Andrii Nakryiko Cc: Martin KaFai Lau, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 23:16 ` Alexei Starovoitov @ 2020-05-22 0:06 ` Martin KaFai Lau 2020-05-26 17:47 ` Andrii Nakryiko 1 sibling, 0 replies; 10+ messages in thread From: Martin KaFai Lau @ 2020-05-22 0:06 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries 2020-05-21 23:16 ` Alexei Starovoitov 2020-05-22 0:06 ` Martin KaFai Lau @ 2020-05-26 17:47 ` Andrii Nakryiko 1 sibling, 0 replies; 10+ messages in thread From: Andrii Nakryiko @ 2020-05-26 17:47 UTC (permalink / raw) To: Alexei Starovoitov Cc: Martin KaFai Lau, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-26 17:47 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-21 19:17 [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Martin KaFai Lau 2020-05-21 19:17 ` [PATCH bpf-next 1/3] bpf: Clean up inner map type check Martin KaFai Lau 2020-05-21 19:18 ` [PATCH bpf-next 2/3] bpf: Relax the max_entries check for inner map Martin KaFai Lau 2020-05-21 19:18 ` [PATCH bpf-next 3/3] bpf: selftests: Add test for different inner map size Martin KaFai Lau 2020-05-21 22:39 ` [PATCH bpf-next 0/3] bpf: Allow inner map with different max_entries Andrii Nakryiko 2020-05-21 22:59 ` Martin KaFai Lau 2020-05-21 23:10 ` Andrii Nakryiko 2020-05-21 23:16 ` Alexei Starovoitov 2020-05-22 0:06 ` Martin KaFai Lau 2020-05-26 17:47 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).