diff mbox series

[v2,bpf-next,3/5] bpf: Support access to bpf map fields

Message ID 6479686a0cd1e9067993df57b4c3eef0e276fec9.1592600985.git.rdna@fb.com
State Accepted
Delegated to: BPF Maintainers
Headers show
Series bpf: Support access to bpf map fields | expand

Commit Message

Andrey Ignatov June 19, 2020, 9:11 p.m. UTC 
There are multiple use-cases when it's convenient to have access to bpf
map fields, both `struct bpf_map` and map type specific struct-s such as
`struct bpf_array`, `struct bpf_htab`, etc.

For example while working with sock arrays it can be necessary to
calculate the key based on map->max_entries (some_hash % max_entries).
Currently this is solved by communicating max_entries via "out-of-band"
channel, e.g. via additional map with known key to get info about target
map. That works, but is not very convenient and error-prone while
working with many maps.

In other cases necessary data is dynamic (i.e. unknown at loading time)
and it's impossible to get it at all. For example while working with a
hash table it can be convenient to know how much capacity is already
used (bpf_htab.count.counter for BPF_F_NO_PREALLOC case).

At the same time kernel knows this info and can provide it to bpf
program.

Fill this gap by adding support to access bpf map fields from bpf
program for both `struct bpf_map` and map type specific fields.

Support is implemented via btf_struct_access() so that a user can define
their own `struct bpf_map` or map type specific struct in their program
with only necessary fields and preserve_access_index attribute, cast a
map to this struct and use a field.

For example:

	struct bpf_map {
		__u32 max_entries;
	} __attribute__((preserve_access_index));

	struct bpf_array {
		struct bpf_map map;
		__u32 elem_size;
	} __attribute__((preserve_access_index));

	struct {
		__uint(type, BPF_MAP_TYPE_ARRAY);
		__uint(max_entries, 4);
		__type(key, __u32);
		__type(value, __u32);
	} m_array SEC(".maps");

	SEC("cgroup_skb/egress")
	int cg_skb(void *ctx)
	{
		struct bpf_array *array = (struct bpf_array *)&m_array;
		struct bpf_map *map = (struct bpf_map *)&m_array;

		/* .. use map->max_entries or array->map.max_entries .. */
	}

Similarly to other btf_struct_access() use-cases (e.g. struct tcp_sock
in net/ipv4/bpf_tcp_ca.c) the patch allows access to any fields of
corresponding struct. Only reading from map fields is supported.

For btf_struct_access() to work there should be a way to know btf id of
a struct that corresponds to a map type. To get btf id there should be a
way to get a stringified name of map-specific struct, such as
"bpf_array", "bpf_htab", etc for a map type. Two new fields are added to
`struct bpf_map_ops` to handle it:
* .map_btf_name keeps a btf name of a struct returned by map_alloc();
* .map_btf_id is used to cache btf id of that struct.

To make btf ids calculation cheaper they're calculated once while
preparing btf_vmlinux and cached same way as it's done for btf_id field
of `struct bpf_func_proto`

While calculating btf ids, struct names are NOT checked for collision.
Collisions will be checked as a part of the work to prepare btf ids used
in verifier in compile time that should land soon. The only known
collision for `struct bpf_htab` (kernel/bpf/hashtab.c vs
net/core/sock_map.c) was fixed earlier.

Both new fields .map_btf_name and .map_btf_id must be set for a map type
for the feature to work. If neither is set for a map type, verifier will
return ENOTSUPP on a try to access map_ptr of corresponding type. If
just one of them set, it's verifier misconfiguration.

Only `struct bpf_array` for BPF_MAP_TYPE_ARRAY and `struct bpf_htab` for
BPF_MAP_TYPE_HASH are supported by this patch. Other map types will be
supported separately.

The feature is available only for CONFIG_DEBUG_INFO_BTF=y and gated by
perfmon_capable() so that unpriv programs won't have access to bpf map
fields.

Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
 include/linux/bpf.h                           |  9 ++
 include/linux/bpf_verifier.h                  |  1 +
 kernel/bpf/arraymap.c                         |  3 +
 kernel/bpf/btf.c                              | 40 +++++++++
 kernel/bpf/hashtab.c                          |  3 +
 kernel/bpf/verifier.c                         | 82 +++++++++++++++++--
 .../selftests/bpf/verifier/map_ptr_mixing.c   |  2 +-
 7 files changed, 131 insertions(+), 9 deletions(-)

Comments

John Fastabend June 21, 2020, 3:27 a.m. UTC | #1 
Andrey Ignatov wrote:
> There are multiple use-cases when it's convenient to have access to bpf
> map fields, both `struct bpf_map` and map type specific struct-s such as
> `struct bpf_array`, `struct bpf_htab`, etc.
> 
> For example while working with sock arrays it can be necessary to
> calculate the key based on map->max_entries (some_hash % max_entries).
> Currently this is solved by communicating max_entries via "out-of-band"
> channel, e.g. via additional map with known key to get info about target
> map. That works, but is not very convenient and error-prone while
> working with many maps.
> 
> In other cases necessary data is dynamic (i.e. unknown at loading time)
> and it's impossible to get it at all. For example while working with a
> hash table it can be convenient to know how much capacity is already
> used (bpf_htab.count.counter for BPF_F_NO_PREALLOC case).
> 
> At the same time kernel knows this info and can provide it to bpf
> program.
> 
> Fill this gap by adding support to access bpf map fields from bpf
> program for both `struct bpf_map` and map type specific fields.
> 
> Support is implemented via btf_struct_access() so that a user can define
> their own `struct bpf_map` or map type specific struct in their program
> with only necessary fields and preserve_access_index attribute, cast a
> map to this struct and use a field.
> 
> For example:
> 
> 	struct bpf_map {
> 		__u32 max_entries;
> 	} __attribute__((preserve_access_index));
> 
> 	struct bpf_array {
> 		struct bpf_map map;
> 		__u32 elem_size;
> 	} __attribute__((preserve_access_index));
> 
> 	struct {
> 		__uint(type, BPF_MAP_TYPE_ARRAY);
> 		__uint(max_entries, 4);
> 		__type(key, __u32);
> 		__type(value, __u32);
> 	} m_array SEC(".maps");
> 
> 	SEC("cgroup_skb/egress")
> 	int cg_skb(void *ctx)
> 	{
> 		struct bpf_array *array = (struct bpf_array *)&m_array;
> 		struct bpf_map *map = (struct bpf_map *)&m_array;
> 
> 		/* .. use map->max_entries or array->map.max_entries .. */
> 	}
> 
> Similarly to other btf_struct_access() use-cases (e.g. struct tcp_sock
> in net/ipv4/bpf_tcp_ca.c) the patch allows access to any fields of
> corresponding struct. Only reading from map fields is supported.
> 
> For btf_struct_access() to work there should be a way to know btf id of
> a struct that corresponds to a map type. To get btf id there should be a
> way to get a stringified name of map-specific struct, such as
> "bpf_array", "bpf_htab", etc for a map type. Two new fields are added to
> `struct bpf_map_ops` to handle it:
> * .map_btf_name keeps a btf name of a struct returned by map_alloc();
> * .map_btf_id is used to cache btf id of that struct.
> 
> To make btf ids calculation cheaper they're calculated once while
> preparing btf_vmlinux and cached same way as it's done for btf_id field
> of `struct bpf_func_proto`
> 
> While calculating btf ids, struct names are NOT checked for collision.
> Collisions will be checked as a part of the work to prepare btf ids used
> in verifier in compile time that should land soon. The only known
> collision for `struct bpf_htab` (kernel/bpf/hashtab.c vs
> net/core/sock_map.c) was fixed earlier.
> 
> Both new fields .map_btf_name and .map_btf_id must be set for a map type
> for the feature to work. If neither is set for a map type, verifier will
> return ENOTSUPP on a try to access map_ptr of corresponding type. If
> just one of them set, it's verifier misconfiguration.
> 
> Only `struct bpf_array` for BPF_MAP_TYPE_ARRAY and `struct bpf_htab` for
> BPF_MAP_TYPE_HASH are supported by this patch. Other map types will be
> supported separately.
> 
> The feature is available only for CONFIG_DEBUG_INFO_BTF=y and gated by
> perfmon_capable() so that unpriv programs won't have access to bpf map
> fields.
> 
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
> ---
>  include/linux/bpf.h                           |  9 ++
>  include/linux/bpf_verifier.h                  |  1 +
>  kernel/bpf/arraymap.c                         |  3 +
>  kernel/bpf/btf.c                              | 40 +++++++++
>  kernel/bpf/hashtab.c                          |  3 +
>  kernel/bpf/verifier.c                         | 82 +++++++++++++++++--
>  .../selftests/bpf/verifier/map_ptr_mixing.c   |  2 +-
>  7 files changed, 131 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 07052d44bca1..1e1501ee53ce 100644

LGTM, but any reason not to allow this with bpf_capable() it looks
useful for building load balancers which might not be related to
CAP_PERFMON.

Otherwise,

Acked-by: John Fastabend <john.fastabend@gmail.com>
Andrey Ignatov June 22, 2020, 6:39 p.m. UTC | #2 
John Fastabend <john.fastabend@gmail.com> [Sat, 2020-06-20 20:27 -0700]:
> Andrey Ignatov wrote:
...
> > 
> > The feature is available only for CONFIG_DEBUG_INFO_BTF=y and gated by
> > perfmon_capable() so that unpriv programs won't have access to bpf map
> > fields.
> > 
> > Signed-off-by: Andrey Ignatov <rdna@fb.com>
> > ---
> >  include/linux/bpf.h                           |  9 ++
> >  include/linux/bpf_verifier.h                  |  1 +
> >  kernel/bpf/arraymap.c                         |  3 +
> >  kernel/bpf/btf.c                              | 40 +++++++++
> >  kernel/bpf/hashtab.c                          |  3 +
> >  kernel/bpf/verifier.c                         | 82 +++++++++++++++++--
> >  .../selftests/bpf/verifier/map_ptr_mixing.c   |  2 +-
> >  7 files changed, 131 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 07052d44bca1..1e1501ee53ce 100644
> 
> LGTM, but any reason not to allow this with bpf_capable() it looks
> useful for building load balancers which might not be related to
> CAP_PERFMON.

Thanks for review John. I agree that this can be useful for many
use-cases, incl. networking programs.

Accessing a kernel struct looks like "tracing" kind of functionality to
me (that's why CAP_PERFMON), but I'm not quite sure, and using
bpf_capable() looks fine as well.

Alexei, since you introduced CAP_BPF, could you clarify which cap is the
right one to use here and why?


> Otherwise,
> 
> Acked-by: John Fastabend <john.fastabend@gmail.com>
Alexei Starovoitov June 22, 2020, 10:11 p.m. UTC | #3 
On Mon, Jun 22, 2020 at 11:39:11AM -0700, Andrey Ignatov wrote:
> John Fastabend <john.fastabend@gmail.com> [Sat, 2020-06-20 20:27 -0700]:
> > Andrey Ignatov wrote:
> ...
> > > 
> > > The feature is available only for CONFIG_DEBUG_INFO_BTF=y and gated by
> > > perfmon_capable() so that unpriv programs won't have access to bpf map
> > > fields.
> > > 
> > > Signed-off-by: Andrey Ignatov <rdna@fb.com>
> > > ---
> > >  include/linux/bpf.h                           |  9 ++
> > >  include/linux/bpf_verifier.h                  |  1 +
> > >  kernel/bpf/arraymap.c                         |  3 +
> > >  kernel/bpf/btf.c                              | 40 +++++++++
> > >  kernel/bpf/hashtab.c                          |  3 +
> > >  kernel/bpf/verifier.c                         | 82 +++++++++++++++++--
> > >  .../selftests/bpf/verifier/map_ptr_mixing.c   |  2 +-
> > >  7 files changed, 131 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 07052d44bca1..1e1501ee53ce 100644
> > 
> > LGTM, but any reason not to allow this with bpf_capable() it looks
> > useful for building load balancers which might not be related to
> > CAP_PERFMON.
> 
> Thanks for review John. I agree that this can be useful for many
> use-cases, incl. networking programs.
> 
> Accessing a kernel struct looks like "tracing" kind of functionality to
> me (that's why CAP_PERFMON), but I'm not quite sure, and using
> bpf_capable() looks fine as well.
> 
> Alexei, since you introduced CAP_BPF, could you clarify which cap is the
> right one to use here and why?

It's tracing and pointers are accessed, so cap_perfmon.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 07052d44bca1..1e1501ee53ce 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -92,6 +92,10 @@  struct bpf_map_ops {
 	int (*map_mmap)(struct bpf_map *map, struct vm_area_struct *vma);
 	__poll_t (*map_poll)(struct bpf_map *map, struct file *filp,
 			     struct poll_table_struct *pts);
+
+	/* BTF name and id of struct allocated by map_alloc */
+	const char * const map_btf_name;
+	int *map_btf_id;
 };
 
 struct bpf_map_memory {
@@ -1109,6 +1113,11 @@  static inline bool bpf_allow_ptr_leaks(void)
 	return perfmon_capable();
 }
 
+static inline bool bpf_allow_ptr_to_map_access(void)
+{
+	return perfmon_capable();
+}
+
 static inline bool bpf_bypass_spec_v1(void)
 {
 	return perfmon_capable();
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index ca08db4ffb5f..53c7bd568c5d 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -379,6 +379,7 @@  struct bpf_verifier_env {
 	u32 used_map_cnt;		/* number of used maps */
 	u32 id_gen;			/* used to generate unique reg IDs */
 	bool allow_ptr_leaks;
+	bool allow_ptr_to_map_access;
 	bool bpf_capable;
 	bool bypass_spec_v1;
 	bool bypass_spec_v4;
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 11584618e861..e7caa48812fb 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -494,6 +494,7 @@  static int array_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
 				   vma->vm_pgoff + pgoff);
 }
 
+static int array_map_btf_id;
 const struct bpf_map_ops array_map_ops = {
 	.map_alloc_check = array_map_alloc_check,
 	.map_alloc = array_map_alloc,
@@ -510,6 +511,8 @@  const struct bpf_map_ops array_map_ops = {
 	.map_check_btf = array_map_check_btf,
 	.map_lookup_batch = generic_map_lookup_batch,
 	.map_update_batch = generic_map_update_batch,
+	.map_btf_name = "bpf_array",
+	.map_btf_id = &array_map_btf_id,
 };
 
 const struct bpf_map_ops percpu_array_map_ops = {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 3eb804618a53..e377d1981730 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3571,6 +3571,41 @@  btf_get_prog_ctx_type(struct bpf_verifier_log *log, struct btf *btf,
 	return ctx_type;
 }
 
+static const struct bpf_map_ops * const btf_vmlinux_map_ops[] = {
+#define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type)
+#define BPF_LINK_TYPE(_id, _name)
+#define BPF_MAP_TYPE(_id, _ops) \
+	[_id] = &_ops,
+#include <linux/bpf_types.h>
+#undef BPF_PROG_TYPE
+#undef BPF_LINK_TYPE
+#undef BPF_MAP_TYPE
+};
+
+static int btf_vmlinux_map_ids_init(const struct btf *btf,
+				    struct bpf_verifier_log *log)
+{
+	const struct bpf_map_ops *ops;
+	int i, btf_id;
+
+	for (i = 0; i < ARRAY_SIZE(btf_vmlinux_map_ops); ++i) {
+		ops = btf_vmlinux_map_ops[i];
+		if (!ops || (!ops->map_btf_name && !ops->map_btf_id))
+			continue;
+		if (!ops->map_btf_name || !ops->map_btf_id) {
+			bpf_log(log, "map type %d is misconfigured\n", i);
+			return -EINVAL;
+		}
+		btf_id = btf_find_by_name_kind(btf, ops->map_btf_name,
+					       BTF_KIND_STRUCT);
+		if (btf_id < 0)
+			return btf_id;
+		*ops->map_btf_id = btf_id;
+	}
+
+	return 0;
+}
+
 static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
 				     struct btf *btf,
 				     const struct btf_type *t,
@@ -3633,6 +3668,11 @@  struct btf *btf_parse_vmlinux(void)
 	/* btf_parse_vmlinux() runs under bpf_verifier_lock */
 	bpf_ctx_convert.t = btf_type_by_id(btf, btf_id);
 
+	/* find bpf map structs for map_ptr access checking */
+	err = btf_vmlinux_map_ids_init(btf, log);
+	if (err < 0)
+		goto errout;
+
 	bpf_struct_ops_init(btf, log);
 
 	btf_verifier_env_free(env);
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index b4b288a3c3c9..2c5999e02060 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1614,6 +1614,7 @@  htab_lru_map_lookup_and_delete_batch(struct bpf_map *map,
 						  true, false);
 }
 
+static int htab_map_btf_id;
 const struct bpf_map_ops htab_map_ops = {
 	.map_alloc_check = htab_map_alloc_check,
 	.map_alloc = htab_map_alloc,
@@ -1625,6 +1626,8 @@  const struct bpf_map_ops htab_map_ops = {
 	.map_gen_lookup = htab_map_gen_lookup,
 	.map_seq_show_elem = htab_map_seq_show_elem,
 	BATCH_OPS(htab),
+	.map_btf_name = "bpf_htab",
+	.map_btf_id = &htab_map_btf_id,
 };
 
 const struct bpf_map_ops htab_lru_map_ops = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 34cde841ab68..20e0637679a7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1351,6 +1351,19 @@  static void mark_reg_not_init(struct bpf_verifier_env *env,
 	__mark_reg_not_init(env, regs + regno);
 }
 
+static void mark_btf_ld_reg(struct bpf_verifier_env *env,
+			    struct bpf_reg_state *regs, u32 regno,
+			    enum bpf_reg_type reg_type, u32 btf_id)
+{
+	if (reg_type == SCALAR_VALUE) {
+		mark_reg_unknown(env, regs, regno);
+		return;
+	}
+	mark_reg_known_zero(env, regs, regno);
+	regs[regno].type = PTR_TO_BTF_ID;
+	regs[regno].btf_id = btf_id;
+}
+
 #define DEF_NOT_SUBREG	(0)
 static void init_reg_state(struct bpf_verifier_env *env,
 			   struct bpf_func_state *state)
@@ -3182,19 +3195,68 @@  static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	if (ret < 0)
 		return ret;
 
-	if (atype == BPF_READ && value_regno >= 0) {
-		if (ret == SCALAR_VALUE) {
-			mark_reg_unknown(env, regs, value_regno);
-			return 0;
-		}
-		mark_reg_known_zero(env, regs, value_regno);
-		regs[value_regno].type = PTR_TO_BTF_ID;
-		regs[value_regno].btf_id = btf_id;
+	if (atype == BPF_READ && value_regno >= 0)
+		mark_btf_ld_reg(env, regs, value_regno, ret, btf_id);
+
+	return 0;
+}
+
+static int check_ptr_to_map_access(struct bpf_verifier_env *env,
+				   struct bpf_reg_state *regs,
+				   int regno, int off, int size,
+				   enum bpf_access_type atype,
+				   int value_regno)
+{
+	struct bpf_reg_state *reg = regs + regno;
+	struct bpf_map *map = reg->map_ptr;
+	const struct btf_type *t;
+	const char *tname;
+	u32 btf_id;
+	int ret;
+
+	if (!btf_vmlinux) {
+		verbose(env, "map_ptr access not supported without CONFIG_DEBUG_INFO_BTF\n");
+		return -ENOTSUPP;
+	}
+
+	if (!map->ops->map_btf_id || !*map->ops->map_btf_id) {
+		verbose(env, "map_ptr access not supported for map type %d\n",
+			map->map_type);
+		return -ENOTSUPP;
+	}
+
+	t = btf_type_by_id(btf_vmlinux, *map->ops->map_btf_id);
+	tname = btf_name_by_offset(btf_vmlinux, t->name_off);
+
+	if (!env->allow_ptr_to_map_access) {
+		verbose(env,
+			"%s access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN\n",
+			tname);
+		return -EPERM;
 	}
 
+	if (off < 0) {
+		verbose(env, "R%d is %s invalid negative access: off=%d\n",
+			regno, tname, off);
+		return -EACCES;
+	}
+
+	if (atype != BPF_READ) {
+		verbose(env, "only read from %s is supported\n", tname);
+		return -EACCES;
+	}
+
+	ret = btf_struct_access(&env->log, t, off, size, atype, &btf_id);
+	if (ret < 0)
+		return ret;
+
+	if (value_regno >= 0)
+		mark_btf_ld_reg(env, regs, value_regno, ret, btf_id);
+
 	return 0;
 }
 
+
 /* check whether memory at (regno + off) is accessible for t = (read | write)
  * if t==write, value_regno is a register which value is stored into memory
  * if t==read, value_regno is a register which will receive the value from memory
@@ -3363,6 +3425,9 @@  static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 	} else if (reg->type == PTR_TO_BTF_ID) {
 		err = check_ptr_to_btf_access(env, regs, regno, off, size, t,
 					      value_regno);
+	} else if (reg->type == CONST_PTR_TO_MAP) {
+		err = check_ptr_to_map_access(env, regs, regno, off, size, t,
+					      value_regno);
 	} else {
 		verbose(env, "R%d invalid mem access '%s'\n", regno,
 			reg_type_str[reg->type]);
@@ -10946,6 +11011,7 @@  int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 		env->strict_alignment = false;
 
 	env->allow_ptr_leaks = bpf_allow_ptr_leaks();
+	env->allow_ptr_to_map_access = bpf_allow_ptr_to_map_access();
 	env->bypass_spec_v1 = bpf_bypass_spec_v1();
 	env->bypass_spec_v4 = bpf_bypass_spec_v4();
 	env->bpf_capable = bpf_capable();
diff --git a/tools/testing/selftests/bpf/verifier/map_ptr_mixing.c b/tools/testing/selftests/bpf/verifier/map_ptr_mixing.c
index cd26ee6b7b1d..1f2b8c4cb26d 100644
--- a/tools/testing/selftests/bpf/verifier/map_ptr_mixing.c
+++ b/tools/testing/selftests/bpf/verifier/map_ptr_mixing.c
@@ -56,7 +56,7 @@ 
 	.fixup_map_in_map = { 16 },
 	.fixup_map_array_48b = { 13 },
 	.result = REJECT,
-	.errstr = "R0 invalid mem access 'map_ptr'",
+	.errstr = "only read from bpf_array is supported",
 },
 {
 	"cond: two branches returning different map pointers for lookup (tail, tail)",