BPF List
 help / color / mirror / Atom feed
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Dave Marchevsky <davemarchevsky@meta.com>
Subject: [PATCH bpf-next v8 04/22] bpf: Populate field_offs for inner_map_meta
Date: Thu, 17 Nov 2022 21:54:12 +0530	[thread overview]
Message-ID: <20221117162430.1213770-5-memxor@gmail.com> (raw)
In-Reply-To: <20221117162430.1213770-1-memxor@gmail.com>

Far too much code simply assumes that both btf_record and btf_field_offs
are set to valid pointers together, or both are unset. They go together
hand in hand as btf_record describes the special fields and
btf_field_offs is compact representation for runtime copying/zeroing.

It is very difficult to make this clear in the code when the only
exception to this universal invariant is inner_map_meta which is used
as reg->map_ptr in the verifier. This is simply a bug waiting to happen,
as in verifier context we cannot easily distinguish if PTR_TO_MAP_VALUE
is coming from an inner map, and if we ever end up using field_offs for
any reason in the future, we will silently ignore the special fields for
inner map case (as NULL is not an error but unset field_offs).

Hence, simply copy field_offs from inner map together with btf_record.

While at it, refactor code to unwind properly on errors with gotos.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/map_in_map.c | 46 ++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
index 74f91048eee3..b3fa03a84334 100644
--- a/kernel/bpf/map_in_map.c
+++ b/kernel/bpf/map_in_map.c
@@ -12,6 +12,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 	struct bpf_map *inner_map, *inner_map_meta;
 	u32 inner_map_meta_size;
 	struct fd f;
+	int ret;
 
 	f = fdget(inner_map_ufd);
 	inner_map = __bpf_map_get(f);
@@ -20,18 +21,18 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 
 	/* Does not support >1 level map-in-map */
 	if (inner_map->inner_map_meta) {
-		fdput(f);
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto put;
 	}
 
 	if (!inner_map->ops->map_meta_equal) {
-		fdput(f);
-		return ERR_PTR(-ENOTSUPP);
+		ret = -ENOTSUPP;
+		goto put;
 	}
 
 	if (btf_record_has_field(inner_map->record, BPF_SPIN_LOCK)) {
-		fdput(f);
-		return ERR_PTR(-ENOTSUPP);
+		ret = -ENOTSUPP;
+		goto put;
 	}
 
 	inner_map_meta_size = sizeof(*inner_map_meta);
@@ -41,8 +42,8 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 
 	inner_map_meta = kzalloc(inner_map_meta_size, GFP_USER);
 	if (!inner_map_meta) {
-		fdput(f);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto put;
 	}
 
 	inner_map_meta->map_type = inner_map->map_type;
@@ -50,17 +51,30 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 	inner_map_meta->value_size = inner_map->value_size;
 	inner_map_meta->map_flags = inner_map->map_flags;
 	inner_map_meta->max_entries = inner_map->max_entries;
+
 	inner_map_meta->record = btf_record_dup(inner_map->record);
 	if (IS_ERR(inner_map_meta->record)) {
-		struct bpf_map *err_ptr = ERR_CAST(inner_map_meta->record);
 		/* btf_record_dup returns NULL or valid pointer in case of
 		 * invalid/empty/valid, but ERR_PTR in case of errors. During
 		 * equality NULL or IS_ERR is equivalent.
 		 */
-		kfree(inner_map_meta);
-		fdput(f);
-		return err_ptr;
+		ret = PTR_ERR(inner_map_meta->record);
+		goto free;
+	}
+
+	if (inner_map_meta->record) {
+		struct btf_field_offs *field_offs;
+		/* If btf_record is !IS_ERR_OR_NULL, then field_offs is always
+		 * valid.
+		 */
+		field_offs = kmemdup(inner_map->field_offs, sizeof(*inner_map->field_offs), GFP_KERNEL | __GFP_NOWARN);
+		if (!field_offs) {
+			ret = -ENOMEM;
+			goto free_rec;
+		}
+		inner_map_meta->field_offs = field_offs;
 	}
+
 	/* It is critical that inner_map btf is set to inner_map_meta btf, as
 	 * the duplicated btf_record's list_head btf_field structs have
 	 * value_rec members which point into the btf_record populated for the
@@ -81,10 +95,18 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
 
 	fdput(f);
 	return inner_map_meta;
+free_rec:
+	btf_record_free(inner_map_meta->record);
+free:
+	kfree(inner_map_meta);
+put:
+	fdput(f);
+	return ERR_PTR(ret);
 }
 
 void bpf_map_meta_free(struct bpf_map *map_meta)
 {
+	kfree(map_meta->field_offs);
 	btf_record_free(map_meta->record);
 	btf_put(map_meta->btf);
 	kfree(map_meta);
-- 
2.38.1


  parent reply	other threads:[~2022-11-17 16:26 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-17 16:24 [PATCH bpf-next v8 00/22] Allocated objects, BPF linked lists Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 01/22] bpf: Fix early return in map_check_btf Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 02/22] bpf: Do btf_record_free outside map_free callback Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 03/22] bpf: Do btf_record_free for outer maps Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` Kumar Kartikeya Dwivedi [this message]
2022-11-17 16:24 ` [PATCH bpf-next v8 05/22] bpf: Introduce allocated objects support Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 06/22] bpf: Recognize lock and list fields in allocated objects Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 07/22] bpf: Verify ownership relationships for user BTF types Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 08/22] bpf: Allow locking bpf_spin_lock in allocated objects Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 09/22] bpf: Allow locking bpf_spin_lock global variables Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 10/22] bpf: Allow locking bpf_spin_lock in inner map values Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 11/22] bpf: Rewrite kfunc argument handling Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 12/22] bpf: Support constant scalar arguments for kfuncs Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 13/22] bpf: Introduce bpf_obj_new Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 14/22] bpf: Introduce bpf_obj_drop Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 15/22] bpf: Permit NULL checking pointer with non-zero fixed offset Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 16/22] bpf: Introduce single ownership BPF linked list API Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 17/22] bpf: Add 'release on unlock' logic for bpf_list_push_{front,back} Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 18/22] selftests/bpf: Add __contains macro to bpf_experimental.h Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 19/22] selftests/bpf: Update spinlock selftest Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 20/22] selftests/bpf: Add failure test cases for spin lock pairing Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 21/22] selftests/bpf: Add BPF linked list API tests Kumar Kartikeya Dwivedi
2022-11-17 16:24 ` [PATCH bpf-next v8 22/22] selftests/bpf: Add BTF sanity tests Kumar Kartikeya Dwivedi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221117162430.1213770-5-memxor@gmail.com \
    --to=memxor@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davemarchevsky@meta.com \
    --cc=martin.lau@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox