* [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback @ 2018-12-08 0:53 Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Roman Gushchin @ 2018-12-08 0:53 UTC (permalink / raw) To: netdev Cc: kernel-team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann If key_type or value_type are of non-trivial data types (e.g. structure or typedef), it's not possible to check them without the additional information, which can't be obtained without a pointer to the btf structure. So, let's pass btf pointer to the map_check_btf() callbacks. Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> --- include/linux/bpf.h | 3 +++ kernel/bpf/arraymap.c | 1 + kernel/bpf/lpm_trie.c | 1 + kernel/bpf/syscall.c | 3 ++- 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index e82b7039fc66..128d93540b23 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -23,6 +23,7 @@ struct bpf_prog; struct bpf_map; struct sock; struct seq_file; +struct btf; struct btf_type; /* map is generic key/value storage optionally accesible by eBPF programs */ @@ -52,6 +53,7 @@ struct bpf_map_ops { void (*map_seq_show_elem)(struct bpf_map *map, void *key, struct seq_file *m); int (*map_check_btf)(const struct bpf_map *map, + const struct btf *btf, const struct btf_type *key_type, const struct btf_type *value_type); }; @@ -126,6 +128,7 @@ static inline bool bpf_map_support_seq_show(const struct bpf_map *map) } int map_check_no_btf(const struct bpf_map *map, + const struct btf *btf, const struct btf_type *key_type, const struct btf_type *value_type); diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index 24583da9ffd1..25632a75d630 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -382,6 +382,7 @@ static void percpu_array_map_seq_show_elem(struct bpf_map *map, void *key, } static int array_map_check_btf(const struct bpf_map *map, + const struct btf *btf, const struct btf_type *key_type, const struct btf_type *value_type) { diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c index bfd4882e1106..abf1002080df 100644 --- a/kernel/bpf/lpm_trie.c +++ b/kernel/bpf/lpm_trie.c @@ -728,6 +728,7 @@ static int trie_get_next_key(struct bpf_map *map, void *_key, void *_next_key) } static int trie_check_btf(const struct bpf_map *map, + const struct btf *btf, const struct btf_type *key_type, const struct btf_type *value_type) { diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index aa05aa38f4a8..7c2e8ab03a34 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -456,6 +456,7 @@ static int bpf_obj_name_cpy(char *dst, const char *src) } int map_check_no_btf(const struct bpf_map *map, + const struct btf *btf, const struct btf_type *key_type, const struct btf_type *value_type) { @@ -478,7 +479,7 @@ static int map_check_btf(const struct bpf_map *map, const struct btf *btf, return -EINVAL; if (map->ops->map_check_btf) - ret = map->ops->map_check_btf(map, key_type, value_type); + ret = map->ops->map_check_btf(map, btf, key_type, value_type); return ret; } -- 2.19.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps 2018-12-08 0:53 [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Roman Gushchin @ 2018-12-08 0:53 ` Roman Gushchin 2018-12-08 3:37 ` Yonghong Song 2018-12-09 1:56 ` Martin Lau 2018-12-08 0:53 ` [PATCH bpf-next 3/3] selftests/bpf: add btf annotations for cgroup_local_storage maps Roman Gushchin 2018-12-09 2:06 ` [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Martin Lau 2 siblings, 2 replies; 8+ messages in thread From: Roman Gushchin @ 2018-12-08 0:53 UTC (permalink / raw) To: netdev Cc: kernel-team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann Implement bpffs pretty printing for cgroup local storage maps (both shared and per-cpu). Output example (captured for tools/testing/selftests/bpf/netcnt_prog.c): Shared: $ cat /sys/fs/bpf/map_2 # WARNING!! The output is for debug purpose only # WARNING!! The output format will change {4294968594,1}: {9999,1039896} Per-cpu: $ cat /sys/fs/bpf/map_1 # WARNING!! The output is for debug purpose only # WARNING!! The output format will change {4294968594,1}: { cpu0: {0,0,0,0,0} cpu1: {0,0,0,0,0} cpu2: {1,104,0,0,0} cpu3: {0,0,0,0,0} } Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> --- include/linux/btf.h | 10 +++++ kernel/bpf/local_storage.c | 90 +++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/include/linux/btf.h b/include/linux/btf.h index 8c2199b5d250..ac67bc4cbfd9 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -5,6 +5,7 @@ #define _LINUX_BTF_H 1 #include <linux/types.h> +#include <uapi/linux/btf.h> struct btf; struct btf_type; @@ -63,4 +64,13 @@ static inline const char *btf_name_by_offset(const struct btf *btf, } #endif +static inline const struct btf_type *btf_orig_type(const struct btf *btf, + const struct btf_type *t) +{ + while (t && BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF) + t = btf_type_by_id(btf, t->type); + + return t; +} + #endif diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c index b65017dead44..7b51fe1aba3c 100644 --- a/kernel/bpf/local_storage.c +++ b/kernel/bpf/local_storage.c @@ -1,11 +1,13 @@ //SPDX-License-Identifier: GPL-2.0 #include <linux/bpf-cgroup.h> #include <linux/bpf.h> +#include <linux/btf.h> #include <linux/bug.h> #include <linux/filter.h> #include <linux/mm.h> #include <linux/rbtree.h> #include <linux/slab.h> +#include <uapi/linux/btf.h> DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); @@ -308,6 +310,91 @@ static int cgroup_storage_delete_elem(struct bpf_map *map, void *key) return -EINVAL; } +static int cgroup_storage_check_btf(const struct bpf_map *map, + const struct btf *btf, + const struct btf_type *key_type, + const struct btf_type *value_type) +{ + const struct btf_type *st, *t; + struct btf_member *m; + + /* Key is expected to be of struct bpf_cgroup_storage_key type, + * which is: + * struct bpf_cgroup_storage_key { + * __u64 cgroup_inode_id; + * __u32 attach_type; + * }; + */ + + /* + * Key_type must be a structure (or a typedef of a structure) with + * two members. + */ + st = btf_orig_type(btf, key_type); + if (BTF_INFO_KIND(st->info) != BTF_KIND_STRUCT || + BTF_INFO_VLEN(st->info) != 2) + return -EINVAL; + + /* + * The first field must be a 64 bit integer at 0 offset. + */ + m = (struct btf_member *)(st + 1); + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || m->offset || + t->size != + FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id)) + return -EINVAL; + + /* + * The second field must be a 32 bit integer at 0 offset. + */ + m = m + 1; + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || + m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) * + BITS_PER_BYTE || t->size != + FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type)) + return -EINVAL; + + return 0; +} + +static void cgroup_storage_seq_show_elem(struct bpf_map *map, void *_key, + struct seq_file *m) +{ + enum bpf_cgroup_storage_type stype = cgroup_storage_type(map); + struct bpf_cgroup_storage_key *key = _key; + struct bpf_cgroup_storage *storage; + int cpu; + + rcu_read_lock(); + storage = cgroup_storage_lookup(map_to_storage(map), key, false); + if (!storage) { + rcu_read_unlock(); + return; + } + + btf_type_seq_show(map->btf, map->btf_key_type_id, key, m); + stype = cgroup_storage_type(map); + if (stype == BPF_CGROUP_STORAGE_SHARED) { + seq_puts(m, ": "); + btf_type_seq_show(map->btf, map->btf_value_type_id, + &READ_ONCE(storage->buf)->data[0], m); + seq_puts(m, "\n"); + } else { + seq_puts(m, ": {\n"); + for_each_possible_cpu(cpu) { + seq_printf(m, "\tcpu%d: ", cpu); + btf_type_seq_show(map->btf, map->btf_value_type_id, + per_cpu_ptr(storage->percpu_buf, cpu), + m); + seq_puts(m, "\n"); + } + seq_puts(m, "}\n"); + } + rcu_read_unlock(); +} + const struct bpf_map_ops cgroup_storage_map_ops = { .map_alloc = cgroup_storage_map_alloc, .map_free = cgroup_storage_map_free, @@ -315,7 +402,8 @@ const struct bpf_map_ops cgroup_storage_map_ops = { .map_lookup_elem = cgroup_storage_lookup_elem, .map_update_elem = cgroup_storage_update_elem, .map_delete_elem = cgroup_storage_delete_elem, - .map_check_btf = map_check_no_btf, + .map_check_btf = cgroup_storage_check_btf, + .map_seq_show_elem = cgroup_storage_seq_show_elem, }; int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map) -- 2.19.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin @ 2018-12-08 3:37 ` Yonghong Song 2018-12-10 22:46 ` Roman Gushchin 2018-12-09 1:56 ` Martin Lau 1 sibling, 1 reply; 8+ messages in thread From: Yonghong Song @ 2018-12-08 3:37 UTC (permalink / raw) To: Roman Gushchin, netdev Cc: Kernel Team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann On 12/7/18 4:53 PM, Roman Gushchin wrote: > Implement bpffs pretty printing for cgroup local storage maps > (both shared and per-cpu). > Output example (captured for tools/testing/selftests/bpf/netcnt_prog.c): > > Shared: > $ cat /sys/fs/bpf/map_2 > # WARNING!! The output is for debug purpose only > # WARNING!! The output format will change > {4294968594,1}: {9999,1039896} > > Per-cpu: > $ cat /sys/fs/bpf/map_1 > # WARNING!! The output is for debug purpose only > # WARNING!! The output format will change > {4294968594,1}: { > cpu0: {0,0,0,0,0} > cpu1: {0,0,0,0,0} > cpu2: {1,104,0,0,0} > cpu3: {0,0,0,0,0} > } > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > --- > include/linux/btf.h | 10 +++++ > kernel/bpf/local_storage.c | 90 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 99 insertions(+), 1 deletion(-) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 8c2199b5d250..ac67bc4cbfd9 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -5,6 +5,7 @@ > #define _LINUX_BTF_H 1 > > #include <linux/types.h> > +#include <uapi/linux/btf.h> > > struct btf; > struct btf_type; > @@ -63,4 +64,13 @@ static inline const char *btf_name_by_offset(const struct btf *btf, > } > #endif > > +static inline const struct btf_type *btf_orig_type(const struct btf *btf, > + const struct btf_type *t) > +{ > + while (t && BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF) > + t = btf_type_by_id(btf, t->type); technically, type modifier "const" and "volatile" can apply to member type as well. But these modifiers really don't make sense here. Could you add a comment here to mention that they will be treated as an error since such a programming is not really recommended? > + > + return t; > +} > + > #endif > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c > index b65017dead44..7b51fe1aba3c 100644 > --- a/kernel/bpf/local_storage.c > +++ b/kernel/bpf/local_storage.c > @@ -1,11 +1,13 @@ > //SPDX-License-Identifier: GPL-2.0 > #include <linux/bpf-cgroup.h> > #include <linux/bpf.h> > +#include <linux/btf.h> > #include <linux/bug.h> > #include <linux/filter.h> > #include <linux/mm.h> > #include <linux/rbtree.h> > #include <linux/slab.h> > +#include <uapi/linux/btf.h> > > DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); > > @@ -308,6 +310,91 @@ static int cgroup_storage_delete_elem(struct bpf_map *map, void *key) > return -EINVAL; > } > > +static int cgroup_storage_check_btf(const struct bpf_map *map, > + const struct btf *btf, > + const struct btf_type *key_type, > + const struct btf_type *value_type) > +{ > + const struct btf_type *st, *t; > + struct btf_member *m; > + > + /* Key is expected to be of struct bpf_cgroup_storage_key type, > + * which is: > + * struct bpf_cgroup_storage_key { > + * __u64 cgroup_inode_id; > + * __u32 attach_type; > + * }; > + */ > + > + /* > + * Key_type must be a structure (or a typedef of a structure) with > + * two members. > + */ > + st = btf_orig_type(btf, key_type); > + if (BTF_INFO_KIND(st->info) != BTF_KIND_STRUCT || > + BTF_INFO_VLEN(st->info) != 2) > + return -EINVAL; > + > + /* > + * The first field must be a 64 bit integer at 0 offset. > + */ > + m = (struct btf_member *)(st + 1); > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || m->offset || > + t->size != > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id)) > + return -EINVAL; We should not use t->size here. The "t->size" is the type size, and the real number of bits held by the member is BTF_INT_BITS(...) with the argument of the u32 int value after "t". > + > + /* > + * The second field must be a 32 bit integer at 0 offset. > + */ > + m = m + 1; > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || > + m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) * > + BITS_PER_BYTE || t->size != > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type)) > + return -EINVAL; The same is here. t->size should not be used. BTF_INT_BITS(...) should be used. > + > + return 0; > +} > + > +static void cgroup_storage_seq_show_elem(struct bpf_map *map, void *_key, > + struct seq_file *m) > +{ > + enum bpf_cgroup_storage_type stype = cgroup_storage_type(map); > + struct bpf_cgroup_storage_key *key = _key; > + struct bpf_cgroup_storage *storage; > + int cpu; > + > + rcu_read_lock(); > + storage = cgroup_storage_lookup(map_to_storage(map), key, false); > + if (!storage) { > + rcu_read_unlock(); > + return; > + } > + > + btf_type_seq_show(map->btf, map->btf_key_type_id, key, m); > + stype = cgroup_storage_type(map); > + if (stype == BPF_CGROUP_STORAGE_SHARED) { > + seq_puts(m, ": "); > + btf_type_seq_show(map->btf, map->btf_value_type_id, > + &READ_ONCE(storage->buf)->data[0], m); > + seq_puts(m, "\n"); > + } else { > + seq_puts(m, ": {\n"); > + for_each_possible_cpu(cpu) { > + seq_printf(m, "\tcpu%d: ", cpu); > + btf_type_seq_show(map->btf, map->btf_value_type_id, > + per_cpu_ptr(storage->percpu_buf, cpu), > + m); > + seq_puts(m, "\n"); > + } > + seq_puts(m, "}\n"); > + } > + rcu_read_unlock(); > +} > + > const struct bpf_map_ops cgroup_storage_map_ops = { > .map_alloc = cgroup_storage_map_alloc, > .map_free = cgroup_storage_map_free, > @@ -315,7 +402,8 @@ const struct bpf_map_ops cgroup_storage_map_ops = { > .map_lookup_elem = cgroup_storage_lookup_elem, > .map_update_elem = cgroup_storage_update_elem, > .map_delete_elem = cgroup_storage_delete_elem, > - .map_check_btf = map_check_no_btf, > + .map_check_btf = cgroup_storage_check_btf, > + .map_seq_show_elem = cgroup_storage_seq_show_elem, > }; > > int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps 2018-12-08 3:37 ` Yonghong Song @ 2018-12-10 22:46 ` Roman Gushchin 0 siblings, 0 replies; 8+ messages in thread From: Roman Gushchin @ 2018-12-10 22:46 UTC (permalink / raw) To: Yonghong Song Cc: Roman Gushchin, netdev, Kernel Team, linux-kernel, Alexei Starovoitov, Daniel Borkmann On Fri, Dec 07, 2018 at 07:37:35PM -0800, Yonghong Song wrote: > > > On 12/7/18 4:53 PM, Roman Gushchin wrote: > > Implement bpffs pretty printing for cgroup local storage maps > > (both shared and per-cpu). > > Output example (captured for tools/testing/selftests/bpf/netcnt_prog.c): > > > > Shared: > > $ cat /sys/fs/bpf/map_2 > > # WARNING!! The output is for debug purpose only > > # WARNING!! The output format will change > > {4294968594,1}: {9999,1039896} > > > > Per-cpu: > > $ cat /sys/fs/bpf/map_1 > > # WARNING!! The output is for debug purpose only > > # WARNING!! The output format will change > > {4294968594,1}: { > > cpu0: {0,0,0,0,0} > > cpu1: {0,0,0,0,0} > > cpu2: {1,104,0,0,0} > > cpu3: {0,0,0,0,0} > > } > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > --- > > include/linux/btf.h | 10 +++++ > > kernel/bpf/local_storage.c | 90 +++++++++++++++++++++++++++++++++++++- > > 2 files changed, 99 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 8c2199b5d250..ac67bc4cbfd9 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -5,6 +5,7 @@ > > #define _LINUX_BTF_H 1 > > > > #include <linux/types.h> > > +#include <uapi/linux/btf.h> > > > > struct btf; > > struct btf_type; > > @@ -63,4 +64,13 @@ static inline const char *btf_name_by_offset(const struct btf *btf, > > } > > #endif > > > > +static inline const struct btf_type *btf_orig_type(const struct btf *btf, > > + const struct btf_type *t) > > +{ > > + while (t && BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF) > > + t = btf_type_by_id(btf, t->type); > technically, type modifier "const" and "volatile" can apply to member > type as well. But these modifiers really don't make sense here. > Could you add a comment here to mention that they will be treated > as an error since such a programming is not really recommended? Switched over to btf_type_id_size() based on Martin's suggestion. > > > + > > + return t; > > +} > > + > > #endif > > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c > > index b65017dead44..7b51fe1aba3c 100644 > > --- a/kernel/bpf/local_storage.c > > +++ b/kernel/bpf/local_storage.c > > @@ -1,11 +1,13 @@ > > //SPDX-License-Identifier: GPL-2.0 > > #include <linux/bpf-cgroup.h> > > #include <linux/bpf.h> > > +#include <linux/btf.h> > > #include <linux/bug.h> > > #include <linux/filter.h> > > #include <linux/mm.h> > > #include <linux/rbtree.h> > > #include <linux/slab.h> > > +#include <uapi/linux/btf.h> > > > > DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); > > > > @@ -308,6 +310,91 @@ static int cgroup_storage_delete_elem(struct bpf_map *map, void *key) > > return -EINVAL; > > } > > > > +static int cgroup_storage_check_btf(const struct bpf_map *map, > > + const struct btf *btf, > > + const struct btf_type *key_type, > > + const struct btf_type *value_type) > > +{ > > + const struct btf_type *st, *t; > > + struct btf_member *m; > > + > > + /* Key is expected to be of struct bpf_cgroup_storage_key type, > > + * which is: > > + * struct bpf_cgroup_storage_key { > > + * __u64 cgroup_inode_id; > > + * __u32 attach_type; > > + * }; > > + */ > > + > > + /* > > + * Key_type must be a structure (or a typedef of a structure) with > > + * two members. > > + */ > > + st = btf_orig_type(btf, key_type); > > + if (BTF_INFO_KIND(st->info) != BTF_KIND_STRUCT || > > + BTF_INFO_VLEN(st->info) != 2) > > + return -EINVAL; > > + > > + /* > > + * The first field must be a 64 bit integer at 0 offset. > > + */ > > + m = (struct btf_member *)(st + 1); > > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || m->offset || > > + t->size != > > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id)) > > + return -EINVAL; > > We should not use t->size here. The "t->size" is the type size, and the > real number of bits held by the member is BTF_INT_BITS(...) with the > argument of the u32 int value after "t". > > > + > > + /* > > + * The second field must be a 32 bit integer at 0 offset. > > + */ > > + m = m + 1; > > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || > > + m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) * > > + BITS_PER_BYTE || t->size != > > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type)) > > + return -EINVAL; > > The same is here. t->size should not be used. > BTF_INT_BITS(...) should be used. Fixed in v2, which I'll send soon. Thank you for the review! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin 2018-12-08 3:37 ` Yonghong Song @ 2018-12-09 1:56 ` Martin Lau 2018-12-10 18:18 ` Roman Gushchin 1 sibling, 1 reply; 8+ messages in thread From: Martin Lau @ 2018-12-09 1:56 UTC (permalink / raw) To: Roman Gushchin Cc: netdev, Kernel Team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann On Fri, Dec 07, 2018 at 04:53:14PM -0800, Roman Gushchin wrote: > Implement bpffs pretty printing for cgroup local storage maps > (both shared and per-cpu). > Output example (captured for tools/testing/selftests/bpf/netcnt_prog.c): > > Shared: > $ cat /sys/fs/bpf/map_2 > # WARNING!! The output is for debug purpose only > # WARNING!! The output format will change > {4294968594,1}: {9999,1039896} > > Per-cpu: > $ cat /sys/fs/bpf/map_1 > # WARNING!! The output is for debug purpose only > # WARNING!! The output format will change > {4294968594,1}: { > cpu0: {0,0,0,0,0} > cpu1: {0,0,0,0,0} > cpu2: {1,104,0,0,0} > cpu3: {0,0,0,0,0} > } > > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > --- > include/linux/btf.h | 10 +++++ > kernel/bpf/local_storage.c | 90 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 99 insertions(+), 1 deletion(-) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 8c2199b5d250..ac67bc4cbfd9 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -5,6 +5,7 @@ > #define _LINUX_BTF_H 1 > > #include <linux/types.h> > +#include <uapi/linux/btf.h> > > struct btf; > struct btf_type; > @@ -63,4 +64,13 @@ static inline const char *btf_name_by_offset(const struct btf *btf, > } > #endif > > +static inline const struct btf_type *btf_orig_type(const struct btf *btf, > + const struct btf_type *t) > +{ > + while (t && BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF) > + t = btf_type_by_id(btf, t->type); Only typedef is allowed? Not even const? If that is not the case, can btf_type_id_size() be reused? The "type following" has already been done once and then remembered in the verification time. If cgroup_storage_check_btf() can only allow typedef, please move "btf_orig_type()" to the local_storage.c. > + > + return t; > +} > + > #endif > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c > index b65017dead44..7b51fe1aba3c 100644 > --- a/kernel/bpf/local_storage.c > +++ b/kernel/bpf/local_storage.c > @@ -1,11 +1,13 @@ > //SPDX-License-Identifier: GPL-2.0 > #include <linux/bpf-cgroup.h> > #include <linux/bpf.h> > +#include <linux/btf.h> > #include <linux/bug.h> > #include <linux/filter.h> > #include <linux/mm.h> > #include <linux/rbtree.h> > #include <linux/slab.h> > +#include <uapi/linux/btf.h> > > DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); > > @@ -308,6 +310,91 @@ static int cgroup_storage_delete_elem(struct bpf_map *map, void *key) > return -EINVAL; > } > > +static int cgroup_storage_check_btf(const struct bpf_map *map, > + const struct btf *btf, > + const struct btf_type *key_type, Actually, in "map_check_btf()" (just before cgroup_storage_check_btf() is called), btf_type_id_size() has already been called to get the true size and the resolved type (i.e. BTF_INFO_KIND_STRUCT here) in order to reject "key_size != map->key_size". Hence, the key_type passed to cgroup_storage_check_btf() here will not be KIND_TYPEDEF or KIND_CONST. > + const struct btf_type *value_type) > +{ > + const struct btf_type *st, *t; > + struct btf_member *m; > + > + /* Key is expected to be of struct bpf_cgroup_storage_key type, > + * which is: > + * struct bpf_cgroup_storage_key { > + * __u64 cgroup_inode_id; > + * __u32 attach_type; > + * }; > + */ > + > + /* > + * Key_type must be a structure (or a typedef of a structure) with > + * two members. > + */ > + st = btf_orig_type(btf, key_type); > + if (BTF_INFO_KIND(st->info) != BTF_KIND_STRUCT || > + BTF_INFO_VLEN(st->info) != 2) > + return -EINVAL; > + > + /* > + * The first field must be a 64 bit integer at 0 offset. > + */ > + m = (struct btf_member *)(st + 1); > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || m->offset || > + t->size != > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id)) Instead of t->size, BTF_INT_BITS() and BTF_INT_OFFSET() need to be checked (please refer to the key_type check in array_map_check_btf()). I think exposing "btf_type_int_is_regular()" from btf.c will be easier here. Actually, add "btf_type_is_reg_int(t, expected_size)" to btf.h and btf.c, like (uncompiled and untested code): /* * Not a bit field and it must be the expected size. */ bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size) { u8 nr_bits, nr_bytes; u32 int_data; if (!btf_type_is_int(t)) return false; int_data = btf_type_int(t); nr_bits = BTF_INT_BITS(int_data); nr_bytes = BITS_ROUNDUP_BYTES(nr_bits); if (BITS_PER_BYTE_MASKED(nr_bits) || BTF_INT_OFFSET(int_data) || nr_bytes != expected_size) return false; return true; } > + return -EINVAL; > + > + /* > + * The second field must be a 32 bit integer at 0 offset. > + */ > + m = m + 1; > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || > + m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) * > + BITS_PER_BYTE || t->size != > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type)) > + return -EINVAL; > + > + return 0; > +} > + > +static void cgroup_storage_seq_show_elem(struct bpf_map *map, void *_key, > + struct seq_file *m) > +{ > + enum bpf_cgroup_storage_type stype = cgroup_storage_type(map); > + struct bpf_cgroup_storage_key *key = _key; > + struct bpf_cgroup_storage *storage; > + int cpu; > + > + rcu_read_lock(); > + storage = cgroup_storage_lookup(map_to_storage(map), key, false); > + if (!storage) { > + rcu_read_unlock(); > + return; > + } > + > + btf_type_seq_show(map->btf, map->btf_key_type_id, key, m); > + stype = cgroup_storage_type(map); > + if (stype == BPF_CGROUP_STORAGE_SHARED) { > + seq_puts(m, ": "); > + btf_type_seq_show(map->btf, map->btf_value_type_id, > + &READ_ONCE(storage->buf)->data[0], m); > + seq_puts(m, "\n"); > + } else { > + seq_puts(m, ": {\n"); > + for_each_possible_cpu(cpu) { > + seq_printf(m, "\tcpu%d: ", cpu); > + btf_type_seq_show(map->btf, map->btf_value_type_id, > + per_cpu_ptr(storage->percpu_buf, cpu), > + m); > + seq_puts(m, "\n"); > + } > + seq_puts(m, "}\n"); > + } > + rcu_read_unlock(); > +} > + > const struct bpf_map_ops cgroup_storage_map_ops = { > .map_alloc = cgroup_storage_map_alloc, > .map_free = cgroup_storage_map_free, > @@ -315,7 +402,8 @@ const struct bpf_map_ops cgroup_storage_map_ops = { > .map_lookup_elem = cgroup_storage_lookup_elem, > .map_update_elem = cgroup_storage_update_elem, > .map_delete_elem = cgroup_storage_delete_elem, > - .map_check_btf = map_check_no_btf, > + .map_check_btf = cgroup_storage_check_btf, > + .map_seq_show_elem = cgroup_storage_seq_show_elem, > }; > > int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *_map) > -- > 2.19.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps 2018-12-09 1:56 ` Martin Lau @ 2018-12-10 18:18 ` Roman Gushchin 0 siblings, 0 replies; 8+ messages in thread From: Roman Gushchin @ 2018-12-10 18:18 UTC (permalink / raw) To: Martin Lau Cc: Roman Gushchin, netdev, Kernel Team, linux-kernel, Alexei Starovoitov, Daniel Borkmann On Sat, Dec 08, 2018 at 05:56:30PM -0800, Martin Lau wrote: > On Fri, Dec 07, 2018 at 04:53:14PM -0800, Roman Gushchin wrote: > > Implement bpffs pretty printing for cgroup local storage maps > > (both shared and per-cpu). > > Output example (captured for tools/testing/selftests/bpf/netcnt_prog.c): > > > > Shared: > > $ cat /sys/fs/bpf/map_2 > > # WARNING!! The output is for debug purpose only > > # WARNING!! The output format will change > > {4294968594,1}: {9999,1039896} > > > > Per-cpu: > > $ cat /sys/fs/bpf/map_1 > > # WARNING!! The output is for debug purpose only > > # WARNING!! The output format will change > > {4294968594,1}: { > > cpu0: {0,0,0,0,0} > > cpu1: {0,0,0,0,0} > > cpu2: {1,104,0,0,0} > > cpu3: {0,0,0,0,0} > > } > > > > Signed-off-by: Roman Gushchin <guro@fb.com> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > --- > > include/linux/btf.h | 10 +++++ > > kernel/bpf/local_storage.c | 90 +++++++++++++++++++++++++++++++++++++- > > 2 files changed, 99 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 8c2199b5d250..ac67bc4cbfd9 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -5,6 +5,7 @@ > > #define _LINUX_BTF_H 1 > > > > #include <linux/types.h> > > +#include <uapi/linux/btf.h> > > > > struct btf; > > struct btf_type; > > @@ -63,4 +64,13 @@ static inline const char *btf_name_by_offset(const struct btf *btf, > > } > > #endif > > > > +static inline const struct btf_type *btf_orig_type(const struct btf *btf, > > + const struct btf_type *t) > > +{ > > + while (t && BTF_INFO_KIND(t->info) == BTF_KIND_TYPEDEF) > > + t = btf_type_by_id(btf, t->type); > Only typedef is allowed? Not even const? > If that is not the case, can btf_type_id_size() be reused? > The "type following" has already been done once and then remembered > in the verification time. > > If cgroup_storage_check_btf() can only allow typedef, please > move "btf_orig_type()" to the local_storage.c. > > > + > > + return t; > > +} > > + > > #endif > > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c > > index b65017dead44..7b51fe1aba3c 100644 > > --- a/kernel/bpf/local_storage.c > > +++ b/kernel/bpf/local_storage.c > > @@ -1,11 +1,13 @@ > > //SPDX-License-Identifier: GPL-2.0 > > #include <linux/bpf-cgroup.h> > > #include <linux/bpf.h> > > +#include <linux/btf.h> > > #include <linux/bug.h> > > #include <linux/filter.h> > > #include <linux/mm.h> > > #include <linux/rbtree.h> > > #include <linux/slab.h> > > +#include <uapi/linux/btf.h> > > > > DEFINE_PER_CPU(struct bpf_cgroup_storage*, bpf_cgroup_storage[MAX_BPF_CGROUP_STORAGE_TYPE]); > > > > @@ -308,6 +310,91 @@ static int cgroup_storage_delete_elem(struct bpf_map *map, void *key) > > return -EINVAL; > > } > > > > +static int cgroup_storage_check_btf(const struct bpf_map *map, > > + const struct btf *btf, > > + const struct btf_type *key_type, > Actually, in "map_check_btf()" (just before cgroup_storage_check_btf() > is called), btf_type_id_size() has already been called > to get the true size and the resolved type (i.e. BTF_INFO_KIND_STRUCT here) > in order to reject "key_size != map->key_size". Hence, the key_type > passed to cgroup_storage_check_btf() here will not be KIND_TYPEDEF or > KIND_CONST. So, the type here is a structure, and its fields are typedefs of ints. Looks like reusing btf_type_id_size() is the best approach. > > > + const struct btf_type *value_type) > > +{ > > + const struct btf_type *st, *t; > > + struct btf_member *m; > > + > > + /* Key is expected to be of struct bpf_cgroup_storage_key type, > > + * which is: > > + * struct bpf_cgroup_storage_key { > > + * __u64 cgroup_inode_id; > > + * __u32 attach_type; > > + * }; > > + */ > > + > > + /* > > + * Key_type must be a structure (or a typedef of a structure) with > > + * two members. > > + */ > > + st = btf_orig_type(btf, key_type); > > + if (BTF_INFO_KIND(st->info) != BTF_KIND_STRUCT || > > + BTF_INFO_VLEN(st->info) != 2) > > + return -EINVAL; > > + > > + /* > > + * The first field must be a 64 bit integer at 0 offset. > > + */ > > + m = (struct btf_member *)(st + 1); > > + t = btf_orig_type(btf, btf_type_by_id(btf, m->type)); > > + if (!t || BTF_INFO_KIND(t->info) != BTF_KIND_INT || m->offset || > > + t->size != > > + FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id)) > Instead of t->size, > BTF_INT_BITS() and BTF_INT_OFFSET() need to be checked (please refer to the > key_type check in array_map_check_btf()). Gotcha. > > I think exposing "btf_type_int_is_regular()" from btf.c will be easier here. > Actually, add "btf_type_is_reg_int(t, expected_size)" to btf.h and btf.c, > like (uncompiled and untested code): > > /* > * Not a bit field and it must be the expected size. > */ > bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size) > { > u8 nr_bits, nr_bytes; > u32 int_data; > > if (!btf_type_is_int(t)) > return false; > > int_data = btf_type_int(t); > nr_bits = BTF_INT_BITS(int_data); > nr_bytes = BITS_ROUNDUP_BYTES(nr_bits); > if (BITS_PER_BYTE_MASKED(nr_bits) || > BTF_INT_OFFSET(int_data) || > nr_bytes != expected_size) > return false; > > return true; > } Looks good to me. Will implement in v2. Thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 3/3] selftests/bpf: add btf annotations for cgroup_local_storage maps 2018-12-08 0:53 [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin @ 2018-12-08 0:53 ` Roman Gushchin 2018-12-09 2:06 ` [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Martin Lau 2 siblings, 0 replies; 8+ messages in thread From: Roman Gushchin @ 2018-12-08 0:53 UTC (permalink / raw) To: netdev Cc: kernel-team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann Add btf annotations to cgroup local storage maps (per-cpu and shared) in the network packet counting example. Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> --- tools/testing/selftests/bpf/netcnt_prog.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c index 1198abca1360..9f741e69cebe 100644 --- a/tools/testing/selftests/bpf/netcnt_prog.c +++ b/tools/testing/selftests/bpf/netcnt_prog.c @@ -16,12 +16,18 @@ struct bpf_map_def SEC("maps") percpu_netcnt = { .value_size = sizeof(struct percpu_net_cnt), }; +BPF_ANNOTATE_KV_PAIR(percpu_netcnt, struct bpf_cgroup_storage_key, + struct percpu_net_cnt); + struct bpf_map_def SEC("maps") netcnt = { .type = BPF_MAP_TYPE_CGROUP_STORAGE, .key_size = sizeof(struct bpf_cgroup_storage_key), .value_size = sizeof(struct net_cnt), }; +BPF_ANNOTATE_KV_PAIR(netcnt, struct bpf_cgroup_storage_key, + struct net_cnt); + SEC("cgroup/skb") int bpf_nextcnt(struct __sk_buff *skb) { -- 2.19.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback 2018-12-08 0:53 [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 3/3] selftests/bpf: add btf annotations for cgroup_local_storage maps Roman Gushchin @ 2018-12-09 2:06 ` Martin Lau 2 siblings, 0 replies; 8+ messages in thread From: Martin Lau @ 2018-12-09 2:06 UTC (permalink / raw) To: Roman Gushchin Cc: netdev, Kernel Team, linux-kernel, Roman Gushchin, Alexei Starovoitov, Daniel Borkmann On Fri, Dec 07, 2018 at 04:53:13PM -0800, Roman Gushchin wrote: > If key_type or value_type are of non-trivial data types > (e.g. structure or typedef), it's not possible to check them without > the additional information, which can't be obtained without a pointer > to the btf structure. > > So, let's pass btf pointer to the map_check_btf() callbacks. Acked-by: Martin KaFai Lau <kafai@fb.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-12-10 22:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-08 0:53 [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 2/3] bpf: add bpffs pretty print for cgroup local storage maps Roman Gushchin 2018-12-08 3:37 ` Yonghong Song 2018-12-10 22:46 ` Roman Gushchin 2018-12-09 1:56 ` Martin Lau 2018-12-10 18:18 ` Roman Gushchin 2018-12-08 0:53 ` [PATCH bpf-next 3/3] selftests/bpf: add btf annotations for cgroup_local_storage maps Roman Gushchin 2018-12-09 2:06 ` [PATCH bpf-next 1/3] bpf: pass struct btf pointer to the map_check_btf() callback Martin Lau
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).