[v1,bpf-next,1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields

Commit Message

Dave Marchevsky Oct. 23, 2023, 10 p.m. UTC

If a struct has a bpf_refcount field, the refcount controls lifetime of
the entire struct. Currently there's no usecase or support for multiple
bpf_refcount fields in a struct.

bpf_spin_lock and bpf_timer fields don't support multiples either, but
with better error behavior. Parsing BTF w/ a struct containing multiple
{bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
the last bpf_refcount field to actually do refcounting.

This patch changes bpf_refcount handling in btf_get_field_type to use
same error logic as bpf_spin_lock and bpf_timer. Since it's being used
3x and is boilerplatey, the logic is shoved into
field_mask_test_name_check_seen helper macro.

Signed-off-by: Dave Marchevsky <[e-mail]>
Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")
---
 kernel/bpf/btf.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

Comments

Yonghong Song Oct. 30, 2023, 5:56 p.m. UTC | #1

On 10/23/23 3:00 PM, Dave Marchevsky wrote:
> If a struct has a bpf_refcount field, the refcount controls lifetime of
> the entire struct. Currently there's no usecase or support for multiple
> bpf_refcount fields in a struct.
>
> bpf_spin_lock and bpf_timer fields don't support multiples either, but
> with better error behavior. Parsing BTF w/ a struct containing multiple
> {bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
> multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
> triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
> the last bpf_refcount field to actually do refcounting.
>
> This patch changes bpf_refcount handling in btf_get_field_type to use
> same error logic as bpf_spin_lock and bpf_timer. Since it's being used
> 3x and is boilerplatey, the logic is shoved into
> field_mask_test_name_check_seen helper macro.
>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")

Acked-by: Yonghong Song <[e-mail]>

Andrii Nakryiko Nov. 1, 2023, 9:56 p.m. UTC | #2

On Mon, Oct 23, 2023 at 3:00 PM Dave Marchevsky <[e-mail]> wrote:
>
> If a struct has a bpf_refcount field, the refcount controls lifetime of
> the entire struct. Currently there's no usecase or support for multiple
> bpf_refcount fields in a struct.
>
> bpf_spin_lock and bpf_timer fields don't support multiples either, but
> with better error behavior. Parsing BTF w/ a struct containing multiple
> {bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
> multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
> triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
> the last bpf_refcount field to actually do refcounting.
>
> This patch changes bpf_refcount handling in btf_get_field_type to use
> same error logic as bpf_spin_lock and bpf_timer. Since it's being used
> 3x and is boilerplatey, the logic is shoved into
> field_mask_test_name_check_seen helper macro.
>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")
> ---
>  kernel/bpf/btf.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
>

LGTM

Acked-by: Andrii Nakryiko <[e-mail]>

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 15d71d2986d3..975ef8e73393 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3374,8 +3374,17 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
>         return BTF_FIELD_FOUND;
>  }
>

[...]

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 15d71d2986d3..975ef8e73393 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3374,8 +3374,17 @@  btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
 	return BTF_FIELD_FOUND;
 }
 
-#define field_mask_test_name(field_type, field_type_str) \
-	if (field_mask & field_type && !strcmp(name, field_type_str)) { \
+#define field_mask_test_name(field_type, field_type_str)		\
+	if (field_mask & field_type && !strcmp(name, field_type_str)) {	\
+		type = field_type;					\
+		goto end;						\
+	}
+
+#define field_mask_test_name_check_seen(field_type, field_type_str)	\
+	if (field_mask & field_type && !strcmp(name, field_type_str)) {	\
+		if (*seen_mask & field_type)				\
+			return -E2BIG;					\
+		*seen_mask |= field_type;				\
 		type = field_type;					\
 		goto end;						\
 	}
@@ -3385,29 +3394,14 @@  static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
 {
 	int type = 0;
 
-	if (field_mask & BPF_SPIN_LOCK) {
-		if (!strcmp(name, "bpf_spin_lock")) {
-			if (*seen_mask & BPF_SPIN_LOCK)
-				return -E2BIG;
-			*seen_mask |= BPF_SPIN_LOCK;
-			type = BPF_SPIN_LOCK;
-			goto end;
-		}
-	}
-	if (field_mask & BPF_TIMER) {
-		if (!strcmp(name, "bpf_timer")) {
-			if (*seen_mask & BPF_TIMER)
-				return -E2BIG;
-			*seen_mask |= BPF_TIMER;
-			type = BPF_TIMER;
-			goto end;
-		}
-	}
+	field_mask_test_name_check_seen(BPF_SPIN_LOCK, "bpf_spin_lock");
+	field_mask_test_name_check_seen(BPF_TIMER,     "bpf_timer");
+	field_mask_test_name_check_seen(BPF_REFCOUNT,  "bpf_refcount");
+
 	field_mask_test_name(BPF_LIST_HEAD, "bpf_list_head");
 	field_mask_test_name(BPF_LIST_NODE, "bpf_list_node");
 	field_mask_test_name(BPF_RB_ROOT,   "bpf_rb_root");
 	field_mask_test_name(BPF_RB_NODE,   "bpf_rb_node");
-	field_mask_test_name(BPF_REFCOUNT,  "bpf_refcount");
 
 	/* Only return BPF_KPTR when all other types with matchable names fail */
 	if (field_mask & BPF_KPTR) {
@@ -3421,6 +3415,7 @@  static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
 	return type;
 }
 
+#undef field_mask_test_name_check_seen
 #undef field_mask_test_name
 
 static int btf_find_struct_field(const struct btf *btf,