diff mbox series

[RFC,bpf-next,v3,06/11] bpf: validate value_type

Message ID 20230920155923.151136-7-thinker.li@gmail.com (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series Registrating struct_ops types from modules | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR pending PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 pending Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 fail Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 fail Logs for veristat
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 fail Logs for test_progs on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1343 this patch: 1343
netdev/cc_maintainers warning 8 maintainers not CCed: jolsa@kernel.org haoluo@google.com daniel@iogearbox.net sdf@google.com john.fastabend@gmail.com yonghong.song@linux.dev netdev@vger.kernel.org kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 1364 this patch: 1364
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1366 this patch: 1366
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 87 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kui-Feng Lee Sept. 20, 2023, 3:59 p.m. UTC 
From: Kui-Feng Lee <thinker.li@gmail.com>

A value_type should has three members; refcnt, state, and data.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

Comments

Martin KaFai Lau Sept. 26, 2023, 1:03 a.m. UTC | #1 
On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> A value_type should has three members; refcnt, state, and data.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index ef8a1edec891..fb684d2ee99d 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>   
>   static const struct btf_type *module_type;
>   
> +static bool check_value_member(struct btf *btf,
> +			       const struct btf_member *member,
> +			       int index,
> +			       const char *value_name,
> +			       const char *name, const char *type_name,
> +			       u16 kind)
> +{
> +	const char *mname, *mtname;
> +	const struct btf_type *mt;
> +	s32 mtype_id;
> +
> +	mname = btf_name_by_offset(btf, member->name_off);
> +	if (!*mname) {
> +		pr_warn("The member %d of %s should have a name\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (strcmp(mname, name)) {
> +		pr_warn("The member %d of %s should be refcnt\n",
> +			index, value_name);
> +		return false;
> +	}
> +	mtype_id = member->type;
> +	mt = btf_type_by_id(btf, mtype_id);
> +	mtname = btf_name_by_offset(btf, mt->name_off);
> +	if (!*mtname) {
> +		pr_warn("The type of the member %d of %s should have a name\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (strcmp(mtname, type_name)) {
> +		pr_warn("The type of the member %d of %s should be refcount_t\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (btf_kind(mt) != kind) {
> +		pr_warn("The type of the member %d of %s should be %d\n",
> +			index, value_name, btf_kind(mt));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool is_valid_value_type(struct btf *btf, s32 value_id,
> +				const char *type_name, const char *value_name)
> +{
> +	const struct btf_member *member;
> +	const struct btf_type *vt;
> +
> +	vt = btf_type_by_id(btf, value_id);
> +	if (btf_vlen(vt) != 3) {
> +		pr_warn("The number of %s's members should be 3, but we get %d\n",
> +			value_name, btf_vlen(vt));
> +		return false;
> +	}
> +	member = btf_type_member(vt);
> +	if (!check_value_member(btf, member, 0, value_name,
> +				"refcnt", "refcount_t", BTF_KIND_TYPEDEF))
> +		return false;
> +	member++;
> +	if (!check_value_member(btf, member, 1, value_name,
> +				"state", "bpf_struct_ops_state",
> +				BTF_KIND_ENUM))
> +		return false;
> +	member++;

I wonder if giving BPF_STRUCT_OPS_COMMON_VALUE a proper struct will make the 
validation cleaner. Like,

struct bpf_struct_ops_common {
	refcount_t refcnt;
	enum bpf_struct_ops_state state;
};

wdyt?

> +	if (!check_value_member(btf, member, 2, value_name,
> +				"data", type_name, BTF_KIND_STRUCT))

Instead of checking name, I think this can directly check with the st_ops->type.

> +		return false;
> +
> +	return true;
> +}
Kui-Feng Lee Sept. 27, 2023, 8:27 p.m. UTC | #2 
On 9/25/23 18:03, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> A value_type should has three members; refcnt, state, and data.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index ef8a1edec891..fb684d2ee99d 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>>   static const struct btf_type *module_type;
>> +static bool check_value_member(struct btf *btf,
>> +                   const struct btf_member *member,
>> +                   int index,
>> +                   const char *value_name,
>> +                   const char *name, const char *type_name,
>> +                   u16 kind)
>> +{
>> +    const char *mname, *mtname;
>> +    const struct btf_type *mt;
>> +    s32 mtype_id;
>> +
>> +    mname = btf_name_by_offset(btf, member->name_off);
>> +    if (!*mname) {
>> +        pr_warn("The member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mname, name)) {
>> +        pr_warn("The member %d of %s should be refcnt\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    mtype_id = member->type;
>> +    mt = btf_type_by_id(btf, mtype_id);
>> +    mtname = btf_name_by_offset(btf, mt->name_off);
>> +    if (!*mtname) {
>> +        pr_warn("The type of the member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mtname, type_name)) {
>> +        pr_warn("The type of the member %d of %s should be 
>> refcount_t\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (btf_kind(mt) != kind) {
>> +        pr_warn("The type of the member %d of %s should be %d\n",
>> +            index, value_name, btf_kind(mt));
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool is_valid_value_type(struct btf *btf, s32 value_id,
>> +                const char *type_name, const char *value_name)
>> +{
>> +    const struct btf_member *member;
>> +    const struct btf_type *vt;
>> +
>> +    vt = btf_type_by_id(btf, value_id);
>> +    if (btf_vlen(vt) != 3) {
>> +        pr_warn("The number of %s's members should be 3, but we get 
>> %d\n",
>> +            value_name, btf_vlen(vt));
>> +        return false;
>> +    }
>> +    member = btf_type_member(vt);
>> +    if (!check_value_member(btf, member, 0, value_name,
>> +                "refcnt", "refcount_t", BTF_KIND_TYPEDEF))
>> +        return false;
>> +    member++;
>> +    if (!check_value_member(btf, member, 1, value_name,
>> +                "state", "bpf_struct_ops_state",
>> +                BTF_KIND_ENUM))
>> +        return false;
>> +    member++;
> 
> I wonder if giving BPF_STRUCT_OPS_COMMON_VALUE a proper struct will make 
> the validation cleaner. Like,
> 
> struct bpf_struct_ops_common {
>      refcount_t refcnt;
>      enum bpf_struct_ops_state state;
> };
> 
> wdyt?

It should work.

> 
>> +    if (!check_value_member(btf, member, 2, value_name,
>> +                "data", type_name, BTF_KIND_STRUCT))
> 
> Instead of checking name, I think this can directly check with the 
> st_ops->type.

Make sense

> 
>> +        return false;
>> +
>> +    return true;
>> +}
> 
>
diff mbox series

Patch

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index ef8a1edec891..fb684d2ee99d 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -99,6 +99,79 @@  const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
 
 static const struct btf_type *module_type;
 
+static bool check_value_member(struct btf *btf,
+			       const struct btf_member *member,
+			       int index,
+			       const char *value_name,
+			       const char *name, const char *type_name,
+			       u16 kind)
+{
+	const char *mname, *mtname;
+	const struct btf_type *mt;
+	s32 mtype_id;
+
+	mname = btf_name_by_offset(btf, member->name_off);
+	if (!*mname) {
+		pr_warn("The member %d of %s should have a name\n",
+			index, value_name);
+		return false;
+	}
+	if (strcmp(mname, name)) {
+		pr_warn("The member %d of %s should be refcnt\n",
+			index, value_name);
+		return false;
+	}
+	mtype_id = member->type;
+	mt = btf_type_by_id(btf, mtype_id);
+	mtname = btf_name_by_offset(btf, mt->name_off);
+	if (!*mtname) {
+		pr_warn("The type of the member %d of %s should have a name\n",
+			index, value_name);
+		return false;
+	}
+	if (strcmp(mtname, type_name)) {
+		pr_warn("The type of the member %d of %s should be refcount_t\n",
+			index, value_name);
+		return false;
+	}
+	if (btf_kind(mt) != kind) {
+		pr_warn("The type of the member %d of %s should be %d\n",
+			index, value_name, btf_kind(mt));
+		return false;
+	}
+
+	return true;
+}
+
+static bool is_valid_value_type(struct btf *btf, s32 value_id,
+				const char *type_name, const char *value_name)
+{
+	const struct btf_member *member;
+	const struct btf_type *vt;
+
+	vt = btf_type_by_id(btf, value_id);
+	if (btf_vlen(vt) != 3) {
+		pr_warn("The number of %s's members should be 3, but we get %d\n",
+			value_name, btf_vlen(vt));
+		return false;
+	}
+	member = btf_type_member(vt);
+	if (!check_value_member(btf, member, 0, value_name,
+				"refcnt", "refcount_t", BTF_KIND_TYPEDEF))
+		return false;
+	member++;
+	if (!check_value_member(btf, member, 1, value_name,
+				"state", "bpf_struct_ops_state",
+				BTF_KIND_ENUM))
+		return false;
+	member++;
+	if (!check_value_member(btf, member, 2, value_name,
+				"data", type_name, BTF_KIND_STRUCT))
+		return false;
+
+	return true;
+}
+
 static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 				    struct btf *btf,
 				    struct bpf_verifier_log *log)
@@ -125,6 +198,8 @@  static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 			value_name);
 		return;
 	}
+	if (!is_valid_value_type(btf, value_id, st_ops->name, value_name))
+		return;
 
 	type_id = btf_find_by_name_kind(btf, st_ops->name,
 					BTF_KIND_STRUCT);