* [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments.
@ 2024-02-02 22:05 thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 1/6] bpf: Allow PTR_TO_BTF_ID even for pointers to int thinker.li
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
With this RFC, I specifically need comments/feeback on the part 2. It
extends PTR_TO_BTF_ID to support pointers to scalar types, enum types,
pointer types and array types. The purpose of extending PTR_TO_BTF_ID
is to support pointers to non-struct types as nullable arguments of
BPF struct_ops programs. Is it a good way to do it? Other options?
Allow passing null pointers to the operators provided by a struct_ops
object. This is an RFC to collect feedbacks/opinions.
The previous discussions against v1 came to the conclusion that the
developer should did it in ".is_valid_access". However, recently, kCFI for
struct_ops has been landed. We found it is possible to provide a generic
way to annotate arguments by adding a suffix after argument names of stub
functions. So, this RFC is resent to present the new idea.
The function pointers that are passed to struct_ops operators (the function
pointers) are always considered reliable until now. They cannot be
null. However, in certain scenarios, it should be possible to pass null
pointers to these operators. For instance, sched_ext may pass a null
pointer in the struct task type to an operator that is provided by its
struct_ops objects.
The proposed solution here is to add PTR_MAYBE_NULL annotations to
arguments and create instances of struct bpf_ctx_arg_aux (arg_info) for
these arguments. These arg_infos will be installed at
prog->aux->ctx_arg_info and will be checked by the BPF verifier when
loading the programs. When a struct_ops program accesses arguments in the
ctx, the verifier will call btf_ctx_access() (through
bpf_verifier_ops->is_valid_access) to verify the access. btf_ctx_access()
will check arg_info and use the information of the matched arg_info to
properly set reg_type.
For nullable arguments, this patch sets an arg_info to label them with
PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This enforces the verifier to
check programs and ensure that they properly check the pointer. The
programs should check if the pointer is null before reading/writing the
pointed memory.
The implementer of a struct_ops should annotate the arguments that can
be null. The implementer should define a stub function (empty) as a
placeholder for each defined operator. The name of a stub function
should be in the pattern "<st_op_type>__<operator name>". For example,
for test_maybe_null of struct bpf_testmod_ops, it's stub function name
should be "bpf_testmod_ops__test_maybe_null". You mark an argument
nullable by suffixing the argument name with "__nullable" at the stub
function. Here is the example in bpf_testmod.c.
static int bpf_testmod_ops__test_maybe_null(int dummy,
struct task_struct *task__nullable,
u32 *scalar__nullable,
u32 (*ar__nullable)[2],
u32 (*ar2__nullable)[])
{
return 0;
}
This means that the argument 1 (2nd) of bpf_testmod_ops->test_maybe_null,
which is a function pointer that can be null. With this annotation, the
verifier will understand how to check programs using this arguments. A BPF
program that implement test_maybe_null should check the pointer to make
sure it is not null before using it. For example,
if (task__nullable)
save_tgid = task__nullable->tgid
Without the check, the verifier will reject the program.
Since we already has stub functions for kCFI, we just reuse these stub
functions with the naming convention mentioned earlier. These stub
functions with the naming convention is only required if there are nullable
arguments to annotate. For functions without nullable arguments, stub
functions are not necessary for the purpose of this patch.
---
Major changes from v3:
- Move the code collecting argument information to prepare_arg_info()
called in the loop in bpf_struct_ops_desc_init().
- Simplify the memory allocation by having separated arg_info for
each member of a struct_ops type.
- Extend PTR_TO_BTF_ID to pointers to scalar types and array types,
not only to struct types.
Major changes from v2:
- Remove dead code.
- Add comments to explain the code itself.
Major changes from v1:
- Annotate arguments by suffixing argument names with "__nullable" at
stub functions.
v3: https://lore.kernel.org/all/20240122212217.1391878-1-thinker.li@gmail.com/
v2: https://lore.kernel.org/all/20240118224922.336006-1-thinker.li@gmail.com/
Kui-Feng Lee (6):
bpf: Allow PTR_TO_BTF_ID even for pointers to int.
bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array
types.
bpf: Remove an unnecessary check.
bpf: add btf pointer to struct bpf_ctx_arg_aux.
bpf: Create argument information for nullable arguments.
selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
include/linux/bpf.h | 18 ++
kernel/bpf/bpf_struct_ops.c | 185 ++++++++++++++++--
kernel/bpf/btf.c | 83 +++++++-
kernel/bpf/verifier.c | 6 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 23 ++-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 10 +
.../prog_tests/test_struct_ops_maybe_null.c | 61 ++++++
.../bpf/progs/struct_ops_maybe_null.c | 41 ++++
.../bpf/progs/struct_ops_maybe_null_fail.c | 98 ++++++++++
9 files changed, 500 insertions(+), 25 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 1/6] bpf: Allow PTR_TO_BTF_ID even for pointers to int.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
@ 2024-02-02 22:05 ` thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types thinker.li
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Pointers to int are always accepted when checking accesses of a context in
btf_ctx_access(). However, we are going to support pointers to scalar types
with PTR_TO_BTF_ID. Changing the order of checking prog->aux->ctx_arg_info
and checking is_int_ptr() enables the extension in the following patches.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/btf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ef380e546952..0847035bba99 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6252,9 +6252,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
*/
return true;
- if (is_int_ptr(btf, t))
- return true;
-
/* this is a pointer to another type */
for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
@@ -6272,6 +6269,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
}
}
+ if (is_int_ptr(btf, t))
+ return true;
+
info->reg_type = PTR_TO_BTF_ID;
if (prog_args_trusted(prog))
info->reg_type |= PTR_TRUSTED;
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 1/6] bpf: Allow PTR_TO_BTF_ID even for pointers to int thinker.li
@ 2024-02-02 22:05 ` thinker.li
2024-02-03 0:52 ` Martin KaFai Lau
2024-02-02 22:05 ` [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check thinker.li
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
The verifier calls btf_struct_access() to check the access for
PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
(including union). We add the support of scalar types and array types.
btf_reloc_array_access() is responsible for relocating the access from the
whole array to an element in the array. That means to adjust the offset
relatively to the start of an element and change the type to the type of
the element. With this relocation, we can check the access against the
element type instead of the array type itself.
After relocation, the struct types, including union types, will continue
the loop of btf_struct_walk(). Other types are treated as scalar types,
including pointers, and return from btf_struct_access().
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 0847035bba99..d3f94d04c69d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
return -EINVAL;
}
+/* Relocate the access relatively to the beginning of an element in an
+ * array.
+ *
+ * The offset is adjusted relatively to the beginning of the element and the
+ * type is adjusted to the type of the element.
+ *
+ * Return NULL for scalar, enum, and pointer type.
+ * Return a btf_type pointer for struct and union.
+ */
+static const struct btf_type *
+btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf *btf,
+ const struct btf_type *t, int *off, int size)
+{
+ const struct btf_type *rt, *elem_type;
+ u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
+ u32 elem_idx;
+
+ rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
+ &total_nelems, &rt_id);
+ if (IS_ERR(rt))
+ return rt;
+ if (btf_type_is_array(rt)) {
+ if (*off >= rt_size) {
+ bpf_log(log, "access out of range of type %s with offset %d and size %u\n",
+ __btf_name_by_offset(btf, t->name_off), *off, rt_size);
+ return ERR_PTR(-EACCES);
+ }
+
+ /* Multi-dimensional arrays are flattened by
+ * __btf_resolve_size(). Check the comment in
+ * btf_struct_walk().
+ */
+ elem_size = rt_size / total_nelems;
+ elem_idx = *off / elem_size;
+ /* Relocate the offset relatively to the start of the
+ * element at elem_idx.
+ */
+ *off -= elem_idx * elem_size;
+ rt = elem_type;
+ rt_size = elem_size;
+ }
+
+ if (btf_type_is_struct(rt))
+ return rt;
+
+ if (*off + size > rt_size) {
+ bpf_log(log, "access beyond the range of type %s with offset %d and size %d\n",
+ __btf_name_by_offset(btf, rt->name_off), *off, size);
+ return ERR_PTR(-EACCES);
+ }
+
+ /* The access is accepted as a scalar. */
+ return NULL;
+}
+
int btf_struct_access(struct bpf_verifier_log *log,
const struct bpf_reg_state *reg,
int off, int size, enum bpf_access_type atype __maybe_unused,
@@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log *log,
}
t = btf_type_by_id(btf, id);
+ t = btf_reloc_array_access(log, btf, t, &off, size);
+ if (IS_ERR(t))
+ return PTR_ERR(t);
+ if (!t)
+ return SCALAR_VALUE;
+
do {
err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 1/6] bpf: Allow PTR_TO_BTF_ID even for pointers to int thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types thinker.li
@ 2024-02-02 22:05 ` thinker.li
2024-02-03 0:46 ` Martin KaFai Lau
2024-02-02 22:05 ` [RFC bpf-next v4 4/6] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
The "i" here is always equal to "btf_type_vlen(t)" since
the "for_each_member()" loop never breaks.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/bpf_struct_ops.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 0decd862dfe0..f98f580de77a 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -189,20 +189,17 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
}
}
- if (i == btf_type_vlen(t)) {
- if (st_ops->init(btf)) {
- pr_warn("Error in init bpf_struct_ops %s\n",
- st_ops->name);
- return -EINVAL;
- } else {
- st_ops_desc->type_id = type_id;
- st_ops_desc->type = t;
- st_ops_desc->value_id = value_id;
- st_ops_desc->value_type = btf_type_by_id(btf,
- value_id);
- }
+ if (st_ops->init(btf)) {
+ pr_warn("Error in init bpf_struct_ops %s\n",
+ st_ops->name);
+ return -EINVAL;
}
+ st_ops_desc->type_id = type_id;
+ st_ops_desc->type = t;
+ st_ops_desc->value_id = value_id;
+ st_ops_desc->value_type = btf_type_by_id(btf, value_id);
+
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 4/6] bpf: add btf pointer to struct bpf_ctx_arg_aux.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
` (2 preceding siblings ...)
2024-02-02 22:05 ` [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check thinker.li
@ 2024-02-02 22:05 ` thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 6/6] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
5 siblings, 0 replies; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Enable the providers to use types defined in a module instead of in the
kernel (btf_vmlinux).
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 1 +
kernel/bpf/btf.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1ebbee1d648e..9a2ee9456989 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1416,6 +1416,7 @@ struct bpf_ctx_arg_aux {
u32 offset;
enum bpf_reg_type reg_type;
u32 btf_id;
+ struct btf *btf;
};
struct btf_mod_pair {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d3f94d04c69d..20d2160b3db5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6263,7 +6263,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
}
info->reg_type = ctx_arg_info->reg_type;
- info->btf = btf_vmlinux;
+ info->btf = ctx_arg_info->btf ? ctx_arg_info->btf : btf_vmlinux;
info->btf_id = ctx_arg_info->btf_id;
return true;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
` (3 preceding siblings ...)
2024-02-02 22:05 ` [RFC bpf-next v4 4/6] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
@ 2024-02-02 22:05 ` thinker.li
2024-02-03 0:40 ` Martin KaFai Lau
2024-02-02 22:05 ` [RFC bpf-next v4 6/6] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
5 siblings, 1 reply; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Collect argument information from the type information of stub functions to
mark arguments of BPF struct_ops programs with PTR_MAYBE_NULL if they are
nullable. A nullable argument is annotated by suffixing "__nullable" at
the argument name of stub function.
For nullable arguments, this patch sets an arg_info to label their reg_type
with PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This makes the verifier
to check programs and ensure that they properly check the pointer. The
programs should check if the pointer is null before accessing the pointed
memory.
The implementer of a struct_ops type should annotate the arguments that can
be null. The implementer should define a stub function (empty) as a
placeholder for each defined operator. The name of a stub function should
be in the pattern "<st_op_type>__<operator name>". For example, for
test_maybe_null of struct bpf_testmod_ops, it's stub function name should
be "bpf_testmod_ops__test_maybe_null". You mark an argument nullable by
suffixing the argument name with "__nullable" at the stub function.
Since we already has stub functions for kCFI, we just reuse these stub
functions with the naming convention mentioned earlier. These stub
functions with the naming convention is only required if there are nullable
arguments to annotate. For functions having not nullable arguments, stub
functions are not necessary for the purpose of this patch.
This patch will prepare a list of struct bpf_ctx_arg_aux, aka arg_info, for
each member field of a struct_ops type. "arg_info" will be assigned to
"prog->aux->ctx_arg_info" of BPF struct_ops programs in
check_struct_ops_btf_id() so that it can be used by btf_ctx_access() later
to set reg_type properly for the verifier.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 17 ++++
kernel/bpf/bpf_struct_ops.c | 166 ++++++++++++++++++++++++++++++++++--
kernel/bpf/btf.c | 14 +++
kernel/bpf/verifier.c | 6 ++
4 files changed, 198 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9a2ee9456989..63ef5cbfd213 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1709,6 +1709,19 @@ struct bpf_struct_ops {
struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
};
+/* Every member of a struct_ops type has an instance even the member is not
+ * an operator (function pointer). The "arg_info" field will be assigned to
+ * prog->aux->arg_info of BPF struct_ops programs to provide the argument
+ * information required by the verifier to verify the program.
+ *
+ * btf_ctx_access() will lookup prog->aux->arg_info to find the
+ * corresponding entry for an given argument.
+ */
+struct bpf_struct_ops_member_arg_info {
+ struct bpf_ctx_arg_aux *arg_info;
+ u32 arg_info_cnt;
+};
+
struct bpf_struct_ops_desc {
struct bpf_struct_ops *st_ops;
@@ -1716,6 +1729,10 @@ struct bpf_struct_ops_desc {
const struct btf_type *value_type;
u32 type_id;
u32 value_id;
+
+ /* Collection of argument information for each member */
+ struct bpf_struct_ops_member_arg_info *member_arg_info;
+ u32 member_arg_info_cnt;
};
enum bpf_struct_ops_state {
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index f98f580de77a..313f6ceabcf4 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -116,17 +116,148 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id,
return true;
}
+#define MAYBE_NULL_SUFFIX "__nullable"
+#define MAX_STUB_NAME 128
+
+static int match_nullable_suffix(const char *name)
+{
+ int suffix_len, len;
+
+ if (!name)
+ return 0;
+
+ suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
+ len = strlen(name);
+ if (len < suffix_len)
+ return 0;
+
+ return !strcmp(name + len - suffix_len, MAYBE_NULL_SUFFIX);
+}
+
+/* Return the type info of a stub function, if it exists.
+ *
+ * The name of the stub function is made up of the name of the struct_ops
+ * and the name of the function pointer member, separated by "__". For
+ * example, if the struct_ops is named "foo_ops" and the function pointer
+ * member is named "bar", the stub function name would be "foo_ops__bar".
+ */
+static const struct btf_type *
+find_stub_func_proto(struct btf *btf, const char *st_op_name,
+ const char *member_name)
+{
+ char stub_func_name[MAX_STUB_NAME];
+ const struct btf_type *t, *func_proto;
+ s32 btf_id;
+
+ snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s",
+ st_op_name, member_name);
+ btf_id = btf_find_by_name_kind(btf, stub_func_name, BTF_KIND_FUNC);
+ if (btf_id < 0)
+ return NULL;
+ t = btf_type_by_id(btf, btf_id);
+ if (!t)
+ return NULL;
+ func_proto = btf_type_by_id(btf, t->type);
+
+ return func_proto;
+}
+
+/* Prepare argument info for every nullable argument of a member of a
+ * struct_ops type.
+ *
+ * Create and initialize a list of struct bpf_struct_ops_member_arg_info
+ * according to type info of the arguments of the stub functions. (Check
+ * kCFI for more information about stub functions.)
+ *
+ * Each member in the struct_ops type has a struct
+ * bpf_struct_ops_member_arg_info to provide an array of struct
+ * bpf_ctx_arg_aux, which in turn provides the information that used by the
+ * verifier to check the arguments of the BPF struct_ops program assigned
+ * to the member. Here, we only care about the arguments that are marked as
+ * __nullable.
+ *
+ * The array of struct bpf_ctx_arg_aux is eventually assigned to
+ * prog->aux->ctx_arg_info of BPF struct_ops programs and passed to the
+ * verifier. (See check_struct_ops_btf_id())
+ */
+static int prepare_arg_info(struct btf *btf,
+ const char *st_ops_name,
+ const char *member_name,
+ struct bpf_struct_ops_member_arg_info *member_arg_info)
+{
+ const struct btf_type *stub_func_proto, *ptr_type;
+ struct bpf_ctx_arg_aux *arg_info, *ai_buf = NULL;
+ const struct btf_param *args;
+ u32 nargs, arg_no = 0;
+ const char *arg_name;
+ s32 arg_btf_id;
+
+ stub_func_proto = find_stub_func_proto(btf, st_ops_name, member_name);
+ if (!stub_func_proto)
+ return 0;
+
+ nargs = btf_type_vlen(stub_func_proto);
+ if (nargs > MAX_BPF_FUNC_REG_ARGS) {
+ pr_warn("Cannot support #%u args in stub func %s_stub_%s\n",
+ nargs, st_ops_name, member_name);
+ return -EINVAL;
+ }
+
+ ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
+ if (!ai_buf)
+ return -ENOMEM;
+
+ args = btf_params(stub_func_proto);
+ for (arg_no = 0; arg_no < nargs; arg_no++) {
+ /* Skip arguments that is not suffixed with
+ * "__nullable".
+ */
+ arg_name = btf_name_by_offset(btf,
+ args[arg_no].name_off);
+ if (!match_nullable_suffix(arg_name))
+ continue;
+
+ /* Should be a pointer to struct, array, scalar, or enum */
+ ptr_type = btf_type_resolve_ptr(btf, args[arg_no].type,
+ &arg_btf_id);
+ if (!ptr_type ||
+ (!btf_type_is_struct(ptr_type) &&
+ !btf_type_is_array(ptr_type) &&
+ !btf_type_is_scalar(ptr_type) &&
+ !btf_is_any_enum(ptr_type))) {
+ kfree(ai_buf);
+ return -EINVAL;
+ }
+
+ /* Fill the information of the new argument */
+ arg_info = ai_buf + member_arg_info->arg_info_cnt++;
+ arg_info->reg_type =
+ PTR_TRUSTED | PTR_MAYBE_NULL | PTR_TO_BTF_ID;
+ arg_info->btf_id = arg_btf_id;
+ arg_info->btf = btf;
+ arg_info->offset = arg_no * sizeof(u64);
+ }
+
+ if (!member_arg_info->arg_info_cnt)
+ kfree(ai_buf);
+ else
+ member_arg_info->arg_info = ai_buf;
+
+ return 0;
+}
+
int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
struct btf *btf,
struct bpf_verifier_log *log)
{
+ struct bpf_struct_ops_member_arg_info *member_arg_info;
struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
const struct btf_member *member;
const struct btf_type *t;
s32 type_id, value_id;
char value_name[128];
const char *mname;
- int i;
+ int i, err;
if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
sizeof(value_name)) {
@@ -160,6 +291,11 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (!is_valid_value_type(btf, value_id, t, value_name))
return -EINVAL;
+ member_arg_info = kcalloc(btf_type_vlen(t), sizeof(*member_arg_info),
+ GFP_KERNEL);
+ if (!member_arg_info)
+ return -ENOMEM;
+
for_each_member(i, t, member) {
const struct btf_type *func_proto;
@@ -167,13 +303,15 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (!*mname) {
pr_warn("anon member in struct %s is not supported\n",
st_ops->name);
- return -EOPNOTSUPP;
+ err = -EOPNOTSUPP;
+ goto errout;
}
if (__btf_member_bitfield_size(t, member)) {
pr_warn("bit field member %s in struct %s is not supported\n",
mname, st_ops->name);
- return -EOPNOTSUPP;
+ err = -EOPNOTSUPP;
+ goto errout;
}
func_proto = btf_type_resolve_func_ptr(btf,
@@ -185,14 +323,24 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
&st_ops->func_models[i])) {
pr_warn("Error in parsing func ptr %s in struct %s\n",
mname, st_ops->name);
- return -EINVAL;
+ err = -EINVAL;
+ goto errout;
}
+
+ err = prepare_arg_info(btf, st_ops->name, mname,
+ member_arg_info + i);
+ if (err)
+ goto errout;
}
+ st_ops_desc->member_arg_info = member_arg_info;
+ st_ops_desc->member_arg_info_cnt = btf_type_vlen(t);
+
if (st_ops->init(btf)) {
pr_warn("Error in init bpf_struct_ops %s\n",
st_ops->name);
- return -EINVAL;
+ err = -EINVAL;
+ goto errout;
}
st_ops_desc->type_id = type_id;
@@ -201,6 +349,14 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
st_ops_desc->value_type = btf_type_by_id(btf, value_id);
return 0;
+
+errout:
+ while (i > 0)
+ kfree(member_arg_info[--i].arg_info);
+ kfree(member_arg_info);
+ st_ops_desc->member_arg_info = NULL;
+
+ return err;
}
static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 20d2160b3db5..fd192f69eb78 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1699,6 +1699,20 @@ static void btf_free_struct_meta_tab(struct btf *btf)
static void btf_free_struct_ops_tab(struct btf *btf)
{
struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
+ struct bpf_struct_ops_member_arg_info *ma_info;
+ int i, j;
+ u32 cnt;
+
+ if (tab)
+ for (i = 0; i < tab->cnt; i++) {
+ ma_info = tab->ops[i].member_arg_info;
+ if (ma_info) {
+ cnt = tab->ops[i].member_arg_info_cnt;
+ for (j = 0; j < cnt; j++)
+ kfree(ma_info[j].arg_info);
+ }
+ kfree(ma_info);
+ }
kfree(tab);
btf->struct_ops_tab = NULL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cd4d780e5400..d1d1c2836bc2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20373,6 +20373,12 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
}
}
+ /* btf_ctx_access() used this to provide argument type info */
+ prog->aux->ctx_arg_info =
+ st_ops_desc->member_arg_info[member_idx].arg_info;
+ prog->aux->ctx_arg_info_size =
+ st_ops_desc->member_arg_info[member_idx].arg_info_cnt;
+
prog->aux->attach_func_proto = func_proto;
prog->aux->attach_func_name = mname;
env->ops = st_ops->verifier_ops;
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC bpf-next v4 6/6] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
` (4 preceding siblings ...)
2024-02-02 22:05 ` [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments thinker.li
@ 2024-02-02 22:05 ` thinker.li
5 siblings, 0 replies; 15+ messages in thread
From: thinker.li @ 2024-02-02 22:05 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, davemarchevsky,
dvernet
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Test if the verifier verifies nullable pointer arguments correctly for BPF
struct_ops programs.
"test_maybe_null" in struct bpf_testmod_ops is the operator defined for the
test cases here. It has several pointer arguments to various types. These
pointers are majorly classified to 3 categories; pointers to struct types,
pointers to scalar types, and pointers to array types. They are handled
sightly differently.
A BPF program should check a pointer for NULL beforehand to access the
value pointed by the nullable pointer arguments, or the verifier should
reject the programs. The test here includes two parts; the programs
checking pointers properly and the programs not checking pointers
beforehand. The test checks if the verifier accepts the programs checking
properly and rejects the programs not checking at all.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 23 ++++-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 10 ++
.../prog_tests/test_struct_ops_maybe_null.c | 61 ++++++++++++
.../bpf/progs/struct_ops_maybe_null.c | 41 ++++++++
.../bpf/progs/struct_ops_maybe_null_fail.c | 98 +++++++++++++++++++
5 files changed, 228 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 4754c662b39f..a81ca9ccf8aa 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -556,7 +556,10 @@ static int bpf_dummy_reg(void *kdata)
struct bpf_testmod_ops *ops = kdata;
int r;
- r = ops->test_2(4, 3);
+ if (ops->test_maybe_null)
+ r = ops->test_maybe_null(0, NULL, NULL, NULL, NULL);
+ else
+ r = ops->test_2(4, 3);
return 0;
}
@@ -565,19 +568,29 @@ static void bpf_dummy_unreg(void *kdata)
{
}
-static int bpf_testmod_test_1(void)
+static int bpf_testmod_ops__test_1(void)
{
return 0;
}
-static int bpf_testmod_test_2(int a, int b)
+static int bpf_testmod_ops__test_2(int a, int b)
+{
+ return 0;
+}
+
+static int bpf_testmod_ops__test_maybe_null(int dummy,
+ struct task_struct *task__nullable,
+ u32 *scalar__nullable,
+ u32 (*ar__nullable)[2],
+ u32 (*ar2__nullable)[])
{
return 0;
}
static struct bpf_testmod_ops __bpf_testmod_ops = {
- .test_1 = bpf_testmod_test_1,
- .test_2 = bpf_testmod_test_2,
+ .test_1 = bpf_testmod_ops__test_1,
+ .test_2 = bpf_testmod_ops__test_2,
+ .test_maybe_null = bpf_testmod_ops__test_maybe_null,
};
struct bpf_struct_ops bpf_bpf_testmod_ops = {
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index ca5435751c79..845c04ba66c1 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -5,6 +5,8 @@
#include <linux/types.h>
+struct task_struct;
+
struct bpf_testmod_test_read_ctx {
char *buf;
loff_t off;
@@ -28,9 +30,17 @@ struct bpf_iter_testmod_seq {
int cnt;
};
+typedef u32 (*ar_t)[2];
+typedef u32 (*ar2_t)[];
+
struct bpf_testmod_ops {
int (*test_1)(void);
int (*test_2)(int a, int b);
+ /* Used to test nullable arguments. */
+ int (*test_maybe_null)(int dummy, struct task_struct *task,
+ u32 *scalar,
+ ar_t ar,
+ ar2_t ar2);
};
#endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
new file mode 100644
index 000000000000..10f5f4fee407
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "struct_ops_maybe_null.skel.h"
+#include "struct_ops_maybe_null_fail.skel.h"
+
+/* Test that the verifier accepts a program that access a nullable pointer
+ * with a proper check.
+ */
+static void maybe_null(void)
+{
+ struct struct_ops_maybe_null *skel;
+
+ skel = struct_ops_maybe_null__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_and_load"))
+ return;
+
+ struct_ops_maybe_null__destroy(skel);
+}
+
+/* Test that the verifier rejects a program that access a nullable pointer
+ * without a check beforehand.
+ */
+static void maybe_null_fail(void)
+{
+ struct bpf_link *link_1 = NULL, *link_2 = NULL,
+ *link_3 = NULL, *link_4 = NULL;
+ struct struct_ops_maybe_null_fail *skel;
+
+ skel = struct_ops_maybe_null_fail__open();
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_fail__open"))
+ return;
+
+ link_1 = bpf_map__attach_struct_ops(skel->maps.testmod_struct_ptr);
+ ASSERT_ERR_PTR(link_1, "bpf_map__attach_struct_ops struct_ptr");
+
+ link_2 = bpf_map__attach_struct_ops(skel->maps.testmod_scalar_ptr);
+ ASSERT_ERR_PTR(link_2, "bpf_map__attach_struct_ops scalar_ptr");
+
+ link_3 = bpf_map__attach_struct_ops(skel->maps.testmod_array_ptr);
+ ASSERT_ERR_PTR(link_3, "bpf_map__attach_struct_ops array_ptr");
+
+ link_4 = bpf_map__attach_struct_ops(skel->maps.testmod_var_array_ptr);
+ ASSERT_ERR_PTR(link_4, "bpf_map__attach_struct_ops var_array_ptr");
+
+ bpf_link__destroy(link_1);
+ bpf_link__destroy(link_2);
+ bpf_link__destroy(link_3);
+ bpf_link__destroy(link_4);
+ struct_ops_maybe_null_fail__destroy(skel);
+}
+
+void test_struct_ops_maybe_null(void)
+{
+ if (test__start_subtest("maybe_null"))
+ maybe_null();
+ if (test__start_subtest("maybe_null_fail"))
+ maybe_null_fail();
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
new file mode 100644
index 000000000000..9025570f574c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+u64 tgid = 0;
+u32 scalar_value = 0;
+
+/* This is a test BPF program that uses struct_ops to access an argument
+ * that may be NULL. This is a test for the verifier to ensure that it can
+ * rip PTR_MAYBE_NULL correctly. There are tree pointers; task, scalar, and
+ * ar. They are used to test the cases of PTR_TO_BTF_ID, PTR_TO_BUF, and array.
+ */
+SEC("struct_ops/test_maybe_null")
+int BPF_PROG(test_maybe_null, int dummy,
+ struct task_struct *task,
+ u32 *scalar,
+ u32 (*ar)[2],
+ u32 (*ar2)[])
+{
+ if (task)
+ tgid = task->tgid;
+
+ if (scalar)
+ scalar_value = *scalar;
+
+ if (*ar)
+ scalar_value += (*ar)[0];
+
+ return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+ .test_maybe_null = (void *)test_maybe_null,
+};
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
new file mode 100644
index 000000000000..cbb46fc9f02f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int tgid = 0;
+u32 scalar_value = 0;
+
+/* These are test BPF struct_ops programs that demonstrates the access of
+ * an argument that may be NULL. These test programs are used to ensure
+ * that the verifier correctly catches the case where a pointer is not
+ * checked for NULL before dereferencing it.
+ */
+
+/* Test for pointer to a struct type. */
+SEC("struct_ops/test_maybe_null_struct_ptr")
+int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
+ struct task_struct *task,
+ u32 *scalar,
+ u32 (*ar)[2],
+ u32 (*ar2)[])
+{
+ tgid = task->tgid;
+
+ return 0;
+}
+
+/* Test for pointer to a scalar type. */
+SEC("struct_ops/test_maybe_null_scalar_ptr")
+int BPF_PROG(test_maybe_null_scalar_ptr, int dummy,
+ struct task_struct *task,
+ u32 *scalar,
+ u32 (*ar)[2],
+ u32 (*ar2)[])
+{
+ scalar_value = *scalar;
+
+ return 0;
+}
+
+/* Test for pointer to an array type. */
+SEC("struct_ops/test_maybe_null_array_ptr")
+int BPF_PROG(test_maybe_null_array_ptr, int dummy,
+ struct task_struct *task,
+ u32 *scalar,
+ u32 (*ar)[2],
+ u32 (*ar2)[])
+{
+ scalar_value += (*ar)[0];
+ scalar_value += (*ar)[1];
+
+ return 0;
+}
+
+/* Test for pointer to a variable length array type.
+ *
+ * This test program is used to ensure that the verifier correctly rejects
+ * the case that access the content of a variable length array even
+ * checking the pointer for NULL beforhand since the verifier doesn't know
+ * the exact size of the array.
+ */
+SEC("struct_ops/test_maybe_null_var_array_ptr")
+int BPF_PROG(test_maybe_null_var_array_ptr, int dummy,
+ struct task_struct *task,
+ u32 *scalar,
+ u32 (*ar)[2],
+ u32 (*ar2)[])
+{
+ if (ar2)
+ scalar_value += (*ar2)[0];
+
+ return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_struct_ptr = {
+ .test_maybe_null = (void *)test_maybe_null_struct_ptr,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_scalar_ptr = {
+ .test_maybe_null = (void *)test_maybe_null_scalar_ptr,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_array_ptr = {
+ .test_maybe_null = (void *)test_maybe_null_array_ptr,
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_var_array_ptr = {
+ .test_maybe_null = (void *)test_maybe_null_var_array_ptr,
+};
+
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments.
2024-02-02 22:05 ` [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments thinker.li
@ 2024-02-03 0:40 ` Martin KaFai Lau
2024-02-03 1:57 ` Kui-Feng Lee
2024-02-05 1:53 ` Kui-Feng Lee
0 siblings, 2 replies; 15+ messages in thread
From: Martin KaFai Lau @ 2024-02-03 0:40 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii,
davemarchevsky, dvernet
On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Collect argument information from the type information of stub functions to
> mark arguments of BPF struct_ops programs with PTR_MAYBE_NULL if they are
> nullable. A nullable argument is annotated by suffixing "__nullable" at
> the argument name of stub function.
>
> For nullable arguments, this patch sets an arg_info to label their reg_type
> with PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This makes the verifier
> to check programs and ensure that they properly check the pointer. The
> programs should check if the pointer is null before accessing the pointed
> memory.
>
> The implementer of a struct_ops type should annotate the arguments that can
> be null. The implementer should define a stub function (empty) as a
> placeholder for each defined operator. The name of a stub function should
> be in the pattern "<st_op_type>__<operator name>". For example, for
> test_maybe_null of struct bpf_testmod_ops, it's stub function name should
> be "bpf_testmod_ops__test_maybe_null". You mark an argument nullable by
> suffixing the argument name with "__nullable" at the stub function.
>
> Since we already has stub functions for kCFI, we just reuse these stub
> functions with the naming convention mentioned earlier. These stub
> functions with the naming convention is only required if there are nullable
> arguments to annotate. For functions having not nullable arguments, stub
> functions are not necessary for the purpose of this patch.
>
> This patch will prepare a list of struct bpf_ctx_arg_aux, aka arg_info, for
> each member field of a struct_ops type. "arg_info" will be assigned to
> "prog->aux->ctx_arg_info" of BPF struct_ops programs in
> check_struct_ops_btf_id() so that it can be used by btf_ctx_access() later
> to set reg_type properly for the verifier.
I looked at the high level. Some comments below.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 17 ++++
> kernel/bpf/bpf_struct_ops.c | 166 ++++++++++++++++++++++++++++++++++--
> kernel/bpf/btf.c | 14 +++
> kernel/bpf/verifier.c | 6 ++
> 4 files changed, 198 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9a2ee9456989..63ef5cbfd213 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1709,6 +1709,19 @@ struct bpf_struct_ops {
> struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
> };
>
> +/* Every member of a struct_ops type has an instance even the member is not
> + * an operator (function pointer). The "arg_info" field will be assigned to
> + * prog->aux->arg_info of BPF struct_ops programs to provide the argument
> + * information required by the verifier to verify the program.
> + *
> + * btf_ctx_access() will lookup prog->aux->arg_info to find the
> + * corresponding entry for an given argument.
> + */
> +struct bpf_struct_ops_member_arg_info {
> + struct bpf_ctx_arg_aux *arg_info;
> + u32 arg_info_cnt;
> +};
> +
> struct bpf_struct_ops_desc {
> struct bpf_struct_ops *st_ops;
>
> @@ -1716,6 +1729,10 @@ struct bpf_struct_ops_desc {
> const struct btf_type *value_type;
> u32 type_id;
> u32 value_id;
> +
> + /* Collection of argument information for each member */
> + struct bpf_struct_ops_member_arg_info *member_arg_info;
> + u32 member_arg_info_cnt;
> };
>
> enum bpf_struct_ops_state {
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index f98f580de77a..313f6ceabcf4 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -116,17 +116,148 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id,
> return true;
> }
>
> +#define MAYBE_NULL_SUFFIX "__nullable"
> +#define MAX_STUB_NAME 128
> +
> +static int match_nullable_suffix(const char *name)
> +{
> + int suffix_len, len;
> +
> + if (!name)
> + return 0;
> +
> + suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
> + len = strlen(name);
> + if (len < suffix_len)
> + return 0;
> +
> + return !strcmp(name + len - suffix_len, MAYBE_NULL_SUFFIX);
> +}
> +
> +/* Return the type info of a stub function, if it exists.
> + *
> + * The name of the stub function is made up of the name of the struct_ops
> + * and the name of the function pointer member, separated by "__". For
> + * example, if the struct_ops is named "foo_ops" and the function pointer
> + * member is named "bar", the stub function name would be "foo_ops__bar".
> + */
> +static const struct btf_type *
> +find_stub_func_proto(struct btf *btf, const char *st_op_name,
> + const char *member_name)
> +{
> + char stub_func_name[MAX_STUB_NAME];
> + const struct btf_type *t, *func_proto;
> + s32 btf_id;
> +
> + snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s",
> + st_op_name, member_name);
> + btf_id = btf_find_by_name_kind(btf, stub_func_name, BTF_KIND_FUNC);
> + if (btf_id < 0)
> + return NULL;
> + t = btf_type_by_id(btf, btf_id);
> + if (!t)
> + return NULL;
> + func_proto = btf_type_by_id(btf, t->type);
> +
> + return func_proto;
> +}
> +
> +/* Prepare argument info for every nullable argument of a member of a
> + * struct_ops type.
> + *
> + * Create and initialize a list of struct bpf_struct_ops_member_arg_info
> + * according to type info of the arguments of the stub functions. (Check
> + * kCFI for more information about stub functions.)
> + *
> + * Each member in the struct_ops type has a struct
> + * bpf_struct_ops_member_arg_info to provide an array of struct
> + * bpf_ctx_arg_aux, which in turn provides the information that used by the
> + * verifier to check the arguments of the BPF struct_ops program assigned
> + * to the member. Here, we only care about the arguments that are marked as
> + * __nullable.
> + *
> + * The array of struct bpf_ctx_arg_aux is eventually assigned to
> + * prog->aux->ctx_arg_info of BPF struct_ops programs and passed to the
> + * verifier. (See check_struct_ops_btf_id())
> + */
> +static int prepare_arg_info(struct btf *btf,
> + const char *st_ops_name,
> + const char *member_name,
> + struct bpf_struct_ops_member_arg_info *member_arg_info)
> +{
> + const struct btf_type *stub_func_proto, *ptr_type;
> + struct bpf_ctx_arg_aux *arg_info, *ai_buf = NULL;
> + const struct btf_param *args;
> + u32 nargs, arg_no = 0;
> + const char *arg_name;
> + s32 arg_btf_id;
> +
> + stub_func_proto = find_stub_func_proto(btf, st_ops_name, member_name);
> + if (!stub_func_proto)
> + return 0;
> +
> + nargs = btf_type_vlen(stub_func_proto);
> + if (nargs > MAX_BPF_FUNC_REG_ARGS) {
Checking MAX_BPF_FUNC_REG_ARGS on the stub_func_proto may not be the right
check. It should have been done on the origin func_proto (i.e. non-stub) when
preparing the func_model in btf_distill_func_proto(). Please double check.
If it needs to do sanity check on nargs of stub_func_proto, a better check is to
ensure the narg of the stub_func_proto is the same as the orig_func_proto
instead. This discrepancy probably should have been complained by the compiler
already but does not harm to check (==) here in case the argument type is
changed and a force cast is used (more below).
> + pr_warn("Cannot support #%u args in stub func %s_stub_%s\n",
> + nargs, st_ops_name, member_name);
> + return -EINVAL;
> + }
> +
> + ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
> + if (!ai_buf)
> + return -ENOMEM;
> +
> + args = btf_params(stub_func_proto);
> + for (arg_no = 0; arg_no < nargs; arg_no++) {
> + /* Skip arguments that is not suffixed with
> + * "__nullable".
> + */
> + arg_name = btf_name_by_offset(btf,
> + args[arg_no].name_off);
> + if (!match_nullable_suffix(arg_name))
I have a question/request.
On top of tagging nullable, can we extend the ctx_arg_info idea here to allow
changing the pointer type?
In particular, take a stub function in bpf_tcp_ca.c:
static u32 bpf_tcp_ca_ssthresh(struct tcp_sock *tp)
{
return 0;
}
Instead of the "struct sock *sk" argument as defined in the tcp_congestion_ops,
the stub function uses "struct tcp_sock *tp'. If we can reuse the ctx_arg_info
idea here, then it can remove the existing way of changing the pointer type from
bpf_tcp_ca_is_valid_access.
> + continue;
> +
> + /* Should be a pointer to struct, array, scalar, or enum */
> + ptr_type = btf_type_resolve_ptr(btf, args[arg_no].type,
> + &arg_btf_id);
> + if (!ptr_type ||
> + (!btf_type_is_struct(ptr_type) &&
> + !btf_type_is_array(ptr_type) &&
> + !btf_type_is_scalar(ptr_type) &&
> + !btf_is_any_enum(ptr_type))) {
> + kfree(ai_buf);
> + return -EINVAL;
> + }
> +
> + /* Fill the information of the new argument */
> + arg_info = ai_buf + member_arg_info->arg_info_cnt++;
> + arg_info->reg_type =
> + PTR_TRUSTED | PTR_MAYBE_NULL | PTR_TO_BTF_ID;
> + arg_info->btf_id = arg_btf_id;
> + arg_info->btf = btf;
> + arg_info->offset = arg_no * sizeof(u64);
I think for the current struct_ops users should be fine to assume sizeof(u64).
The current struct_ops users should only have pointer/scalar argument (meaning
there is no struct passed-by-value argument).
I still think it is better to get it correct for all trampoline supported
argument here. Take a look at 720e6a435194 ("bpf: Allow struct argument in
trampoline based programs") and get_ctx_arg_idx(). It may be easier (not sure if
it is cleaner) to directly store the arg_no into arg_info here but arg_info only
has offset now. Please think about what could be a cleaner way to do it.
> + }
> +
> + if (!member_arg_info->arg_info_cnt)
> + kfree(ai_buf);
> + else
> + member_arg_info->arg_info = ai_buf;
> +
> + return 0;
> +}
> +
> int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> struct btf *btf,
> struct bpf_verifier_log *log)
> {
> + struct bpf_struct_ops_member_arg_info *member_arg_info;
> struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
> const struct btf_member *member;
> const struct btf_type *t;
> s32 type_id, value_id;
> char value_name[128];
> const char *mname;
> - int i;
> + int i, err;
>
> if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
> sizeof(value_name)) {
> @@ -160,6 +291,11 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> if (!is_valid_value_type(btf, value_id, t, value_name))
> return -EINVAL;
>
> + member_arg_info = kcalloc(btf_type_vlen(t), sizeof(*member_arg_info),
> + GFP_KERNEL);
> + if (!member_arg_info)
> + return -ENOMEM;
> +
> for_each_member(i, t, member) {
> const struct btf_type *func_proto;
>
> @@ -167,13 +303,15 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> if (!*mname) {
> pr_warn("anon member in struct %s is not supported\n",
> st_ops->name);
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> + goto errout;
> }
>
> if (__btf_member_bitfield_size(t, member)) {
> pr_warn("bit field member %s in struct %s is not supported\n",
> mname, st_ops->name);
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> + goto errout;
> }
>
> func_proto = btf_type_resolve_func_ptr(btf,
> @@ -185,14 +323,24 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> &st_ops->func_models[i])) {
> pr_warn("Error in parsing func ptr %s in struct %s\n",
> mname, st_ops->name);
> - return -EINVAL;
> + err = -EINVAL;
> + goto errout;
> }
> +
> + err = prepare_arg_info(btf, st_ops->name, mname,
> + member_arg_info + i);
> + if (err)
> + goto errout;
> }
>
> + st_ops_desc->member_arg_info = member_arg_info;
> + st_ops_desc->member_arg_info_cnt = btf_type_vlen(t);
It should be the same as btf_type_vlen(st_ops_desc->type). I would avoid this
duplicated info within the same st_ops_desc.
> +
> if (st_ops->init(btf)) {
> pr_warn("Error in init bpf_struct_ops %s\n",
> st_ops->name);
> - return -EINVAL;
> + err = -EINVAL;
> + goto errout;
> }
>
> st_ops_desc->type_id = type_id;
> @@ -201,6 +349,14 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>
> return 0;
> +
> +errout:
> + while (i > 0)
> + kfree(member_arg_info[--i].arg_info);
> + kfree(member_arg_info);
> + st_ops_desc->member_arg_info = NULL;
> +
> + return err;
> }
>
> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 20d2160b3db5..fd192f69eb78 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1699,6 +1699,20 @@ static void btf_free_struct_meta_tab(struct btf *btf)
> static void btf_free_struct_ops_tab(struct btf *btf)
> {
> struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
> + struct bpf_struct_ops_member_arg_info *ma_info;
> + int i, j;
> + u32 cnt;
> +
> + if (tab)
> + for (i = 0; i < tab->cnt; i++) {
> + ma_info = tab->ops[i].member_arg_info;
> + if (ma_info) {
> + cnt = tab->ops[i].member_arg_info_cnt;
> + for (j = 0; j < cnt; j++)
> + kfree(ma_info[j].arg_info);
> + }
> + kfree(ma_info);
> + }
>
> kfree(tab);
> btf->struct_ops_tab = NULL;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index cd4d780e5400..d1d1c2836bc2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20373,6 +20373,12 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
> }
> }
>
> + /* btf_ctx_access() used this to provide argument type info */
> + prog->aux->ctx_arg_info =
> + st_ops_desc->member_arg_info[member_idx].arg_info;
> + prog->aux->ctx_arg_info_size =
> + st_ops_desc->member_arg_info[member_idx].arg_info_cnt;
> +
> prog->aux->attach_func_proto = func_proto;
> prog->aux->attach_func_name = mname;
> env->ops = st_ops->verifier_ops;
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check.
2024-02-02 22:05 ` [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check thinker.li
@ 2024-02-03 0:46 ` Martin KaFai Lau
2024-02-03 1:03 ` Kui-Feng Lee
0 siblings, 1 reply; 15+ messages in thread
From: Martin KaFai Lau @ 2024-02-03 0:46 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii,
davemarchevsky, dvernet
On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> The "i" here is always equal to "btf_type_vlen(t)" since
> the "for_each_member()" loop never breaks.
It can be separated from the PTR_MAYBE_NULL support set. Please post this as its
own patch without the RFC.
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> kernel/bpf/bpf_struct_ops.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 0decd862dfe0..f98f580de77a 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -189,20 +189,17 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> }
> }
>
> - if (i == btf_type_vlen(t)) {
> - if (st_ops->init(btf)) {
> - pr_warn("Error in init bpf_struct_ops %s\n",
> - st_ops->name);
> - return -EINVAL;
> - } else {
> - st_ops_desc->type_id = type_id;
> - st_ops_desc->type = t;
> - st_ops_desc->value_id = value_id;
> - st_ops_desc->value_type = btf_type_by_id(btf,
> - value_id);
> - }
> + if (st_ops->init(btf)) {
> + pr_warn("Error in init bpf_struct_ops %s\n",
> + st_ops->name);
> + return -EINVAL;
> }
>
> + st_ops_desc->type_id = type_id;
> + st_ops_desc->type = t;
> + st_ops_desc->value_id = value_id;
> + st_ops_desc->value_type = btf_type_by_id(btf, value_id);
> +
> return 0;
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types.
2024-02-02 22:05 ` [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types thinker.li
@ 2024-02-03 0:52 ` Martin KaFai Lau
2024-02-03 1:03 ` Kui-Feng Lee
0 siblings, 1 reply; 15+ messages in thread
From: Martin KaFai Lau @ 2024-02-03 0:52 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii,
davemarchevsky, dvernet
On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> The verifier calls btf_struct_access() to check the access for
> PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
> (including union). We add the support of scalar types and array types.
>
> btf_reloc_array_access() is responsible for relocating the access from the
> whole array to an element in the array. That means to adjust the offset
> relatively to the start of an element and change the type to the type of
> the element. With this relocation, we can check the access against the
> element type instead of the array type itself.
>
> After relocation, the struct types, including union types, will continue
> the loop of btf_struct_walk(). Other types are treated as scalar types,
> including pointers, and return from btf_struct_access().
Unless there is an immediate use case to support PTR_MAYBE_NULL on a non-struct
pointer, I would suggest to separate the other pointer type support from the
current PTR_MAYBE_NULL feature patchset. afaik, they are orthogonal.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 0847035bba99..d3f94d04c69d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> return -EINVAL;
> }
>
> +/* Relocate the access relatively to the beginning of an element in an
> + * array.
> + *
> + * The offset is adjusted relatively to the beginning of the element and the
> + * type is adjusted to the type of the element.
> + *
> + * Return NULL for scalar, enum, and pointer type.
> + * Return a btf_type pointer for struct and union.
> + */
> +static const struct btf_type *
> +btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf *btf,
> + const struct btf_type *t, int *off, int size)
> +{
> + const struct btf_type *rt, *elem_type;
> + u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
> + u32 elem_idx;
> +
> + rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
> + &total_nelems, &rt_id);
> + if (IS_ERR(rt))
> + return rt;
> + if (btf_type_is_array(rt)) {
> + if (*off >= rt_size) {
> + bpf_log(log, "access out of range of type %s with offset %d and size %u\n",
> + __btf_name_by_offset(btf, t->name_off), *off, rt_size);
> + return ERR_PTR(-EACCES);
> + }
> +
> + /* Multi-dimensional arrays are flattened by
> + * __btf_resolve_size(). Check the comment in
> + * btf_struct_walk().
> + */
> + elem_size = rt_size / total_nelems;
> + elem_idx = *off / elem_size;
> + /* Relocate the offset relatively to the start of the
> + * element at elem_idx.
> + */
> + *off -= elem_idx * elem_size;
> + rt = elem_type;
> + rt_size = elem_size;
> + }
> +
> + if (btf_type_is_struct(rt))
> + return rt;
> +
> + if (*off + size > rt_size) {
> + bpf_log(log, "access beyond the range of type %s with offset %d and size %d\n",
> + __btf_name_by_offset(btf, rt->name_off), *off, size);
> + return ERR_PTR(-EACCES);
> + }
> +
> + /* The access is accepted as a scalar. */
> + return NULL;
> +}
> +
> int btf_struct_access(struct bpf_verifier_log *log,
> const struct bpf_reg_state *reg,
> int off, int size, enum bpf_access_type atype __maybe_unused,
> @@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log *log,
> }
>
> t = btf_type_by_id(btf, id);
> + t = btf_reloc_array_access(log, btf, t, &off, size);
> + if (IS_ERR(t))
> + return PTR_ERR(t);
> + if (!t)
> + return SCALAR_VALUE;
> +
> do {
> err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types.
2024-02-03 0:52 ` Martin KaFai Lau
@ 2024-02-03 1:03 ` Kui-Feng Lee
0 siblings, 0 replies; 15+ messages in thread
From: Kui-Feng Lee @ 2024-02-03 1:03 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/2/24 16:52, Martin KaFai Lau wrote:
> On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> The verifier calls btf_struct_access() to check the access for
>> PTR_TO_BTF_ID. btf_struct_access() supported only pointer to struct types
>> (including union). We add the support of scalar types and array types.
>>
>> btf_reloc_array_access() is responsible for relocating the access from
>> the
>> whole array to an element in the array. That means to adjust the offset
>> relatively to the start of an element and change the type to the type of
>> the element. With this relocation, we can check the access against the
>> element type instead of the array type itself.
>>
>> After relocation, the struct types, including union types, will continue
>> the loop of btf_struct_walk(). Other types are treated as scalar types,
>> including pointers, and return from btf_struct_access().
>
> Unless there is an immediate use case to support PTR_MAYBE_NULL on a
> non-struct pointer, I would suggest to separate the other pointer type
> support from the current PTR_MAYBE_NULL feature patchset. afaik, they
> are orthogonal.
Totally agree!
>
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> kernel/bpf/btf.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 61 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 0847035bba99..d3f94d04c69d 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -6590,6 +6590,61 @@ static int btf_struct_walk(struct
>> bpf_verifier_log *log, const struct btf *btf,
>> return -EINVAL;
>> }
>> +/* Relocate the access relatively to the beginning of an element in an
>> + * array.
>> + *
>> + * The offset is adjusted relatively to the beginning of the element
>> and the
>> + * type is adjusted to the type of the element.
>> + *
>> + * Return NULL for scalar, enum, and pointer type.
>> + * Return a btf_type pointer for struct and union.
>> + */
>> +static const struct btf_type *
>> +btf_reloc_array_access(struct bpf_verifier_log *log, const struct btf
>> *btf,
>> + const struct btf_type *t, int *off, int size)
>> +{
>> + const struct btf_type *rt, *elem_type;
>> + u32 rt_size, elem_id, total_nelems, rt_id, elem_size;
>> + u32 elem_idx;
>> +
>> + rt = __btf_resolve_size(btf, t, &rt_size, &elem_type, &elem_id,
>> + &total_nelems, &rt_id);
>> + if (IS_ERR(rt))
>> + return rt;
>> + if (btf_type_is_array(rt)) {
>> + if (*off >= rt_size) {
>> + bpf_log(log, "access out of range of type %s with offset
>> %d and size %u\n",
>> + __btf_name_by_offset(btf, t->name_off), *off, rt_size);
>> + return ERR_PTR(-EACCES);
>> + }
>> +
>> + /* Multi-dimensional arrays are flattened by
>> + * __btf_resolve_size(). Check the comment in
>> + * btf_struct_walk().
>> + */
>> + elem_size = rt_size / total_nelems;
>> + elem_idx = *off / elem_size;
>> + /* Relocate the offset relatively to the start of the
>> + * element at elem_idx.
>> + */
>> + *off -= elem_idx * elem_size;
>> + rt = elem_type;
>> + rt_size = elem_size;
>> + }
>> +
>> + if (btf_type_is_struct(rt))
>> + return rt;
>> +
>> + if (*off + size > rt_size) {
>> + bpf_log(log, "access beyond the range of type %s with offset
>> %d and size %d\n",
>> + __btf_name_by_offset(btf, rt->name_off), *off, size);
>> + return ERR_PTR(-EACCES);
>> + }
>> +
>> + /* The access is accepted as a scalar. */
>> + return NULL;
>> +}
>> +
>> int btf_struct_access(struct bpf_verifier_log *log,
>> const struct bpf_reg_state *reg,
>> int off, int size, enum bpf_access_type atype
>> __maybe_unused,
>> @@ -6625,6 +6680,12 @@ int btf_struct_access(struct bpf_verifier_log
>> *log,
>> }
>> t = btf_type_by_id(btf, id);
>> + t = btf_reloc_array_access(log, btf, t, &off, size);
>> + if (IS_ERR(t))
>> + return PTR_ERR(t);
>> + if (!t)
>> + return SCALAR_VALUE;
>> +
>> do {
>> err = btf_struct_walk(log, btf, t, off, size, &id,
>> &tmp_flag, field_name);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check.
2024-02-03 0:46 ` Martin KaFai Lau
@ 2024-02-03 1:03 ` Kui-Feng Lee
0 siblings, 0 replies; 15+ messages in thread
From: Kui-Feng Lee @ 2024-02-03 1:03 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/2/24 16:46, Martin KaFai Lau wrote:
> On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> The "i" here is always equal to "btf_type_vlen(t)" since
>> the "for_each_member()" loop never breaks.
>
> It can be separated from the PTR_MAYBE_NULL support set. Please post
> this as its own patch without the RFC.
Sure!
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> kernel/bpf/bpf_struct_ops.c | 21 +++++++++------------
>> 1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 0decd862dfe0..f98f580de77a 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -189,20 +189,17 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> }
>> }
>> - if (i == btf_type_vlen(t)) {
>> - if (st_ops->init(btf)) {
>> - pr_warn("Error in init bpf_struct_ops %s\n",
>> - st_ops->name);
>> - return -EINVAL;
>> - } else {
>> - st_ops_desc->type_id = type_id;
>> - st_ops_desc->type = t;
>> - st_ops_desc->value_id = value_id;
>> - st_ops_desc->value_type = btf_type_by_id(btf,
>> - value_id);
>> - }
>> + if (st_ops->init(btf)) {
>> + pr_warn("Error in init bpf_struct_ops %s\n",
>> + st_ops->name);
>> + return -EINVAL;
>> }
>> + st_ops_desc->type_id = type_id;
>> + st_ops_desc->type = t;
>> + st_ops_desc->value_id = value_id;
>> + st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>> +
>> return 0;
>> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments.
2024-02-03 0:40 ` Martin KaFai Lau
@ 2024-02-03 1:57 ` Kui-Feng Lee
2024-02-04 0:21 ` Kui-Feng Lee
2024-02-05 1:53 ` Kui-Feng Lee
1 sibling, 1 reply; 15+ messages in thread
From: Kui-Feng Lee @ 2024-02-03 1:57 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/2/24 16:40, Martin KaFai Lau wrote:
> On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Collect argument information from the type information of stub
>> functions to
>> mark arguments of BPF struct_ops programs with PTR_MAYBE_NULL if they are
>> nullable. A nullable argument is annotated by suffixing "__nullable" at
>> the argument name of stub function.
>>
>> For nullable arguments, this patch sets an arg_info to label their
>> reg_type
>> with PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This makes the
>> verifier
>> to check programs and ensure that they properly check the pointer. The
>> programs should check if the pointer is null before accessing the pointed
>> memory.
>>
>> The implementer of a struct_ops type should annotate the arguments
>> that can
>> be null. The implementer should define a stub function (empty) as a
>> placeholder for each defined operator. The name of a stub function should
>> be in the pattern "<st_op_type>__<operator name>". For example, for
>> test_maybe_null of struct bpf_testmod_ops, it's stub function name should
>> be "bpf_testmod_ops__test_maybe_null". You mark an argument nullable by
>> suffixing the argument name with "__nullable" at the stub function.
>>
>> Since we already has stub functions for kCFI, we just reuse these stub
>> functions with the naming convention mentioned earlier. These stub
>> functions with the naming convention is only required if there are
>> nullable
>> arguments to annotate. For functions having not nullable arguments, stub
>> functions are not necessary for the purpose of this patch.
>>
>> This patch will prepare a list of struct bpf_ctx_arg_aux, aka
>> arg_info, for
>> each member field of a struct_ops type. "arg_info" will be assigned to
>> "prog->aux->ctx_arg_info" of BPF struct_ops programs in
>> check_struct_ops_btf_id() so that it can be used by btf_ctx_access()
>> later
>> to set reg_type properly for the verifier.
>
> I looked at the high level. Some comments below.
>
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 17 ++++
>> kernel/bpf/bpf_struct_ops.c | 166 ++++++++++++++++++++++++++++++++++--
>> kernel/bpf/btf.c | 14 +++
>> kernel/bpf/verifier.c | 6 ++
>> 4 files changed, 198 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 9a2ee9456989..63ef5cbfd213 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1709,6 +1709,19 @@ struct bpf_struct_ops {
>> struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
>> };
>> +/* Every member of a struct_ops type has an instance even the member
>> is not
>> + * an operator (function pointer). The "arg_info" field will be
>> assigned to
>> + * prog->aux->arg_info of BPF struct_ops programs to provide the
>> argument
>> + * information required by the verifier to verify the program.
>> + *
>> + * btf_ctx_access() will lookup prog->aux->arg_info to find the
>> + * corresponding entry for an given argument.
>> + */
>> +struct bpf_struct_ops_member_arg_info {
>> + struct bpf_ctx_arg_aux *arg_info;
>> + u32 arg_info_cnt;
>> +};
>> +
>> struct bpf_struct_ops_desc {
>> struct bpf_struct_ops *st_ops;
>> @@ -1716,6 +1729,10 @@ struct bpf_struct_ops_desc {
>> const struct btf_type *value_type;
>> u32 type_id;
>> u32 value_id;
>> +
>> + /* Collection of argument information for each member */
>> + struct bpf_struct_ops_member_arg_info *member_arg_info;
>> + u32 member_arg_info_cnt;
>> };
>> enum bpf_struct_ops_state {
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index f98f580de77a..313f6ceabcf4 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -116,17 +116,148 @@ static bool is_valid_value_type(struct btf
>> *btf, s32 value_id,
>> return true;
>> }
>> +#define MAYBE_NULL_SUFFIX "__nullable"
>> +#define MAX_STUB_NAME 128
>> +
>> +static int match_nullable_suffix(const char *name)
>> +{
>> + int suffix_len, len;
>> +
>> + if (!name)
>> + return 0;
>> +
>> + suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
>> + len = strlen(name);
>> + if (len < suffix_len)
>> + return 0;
>> +
>> + return !strcmp(name + len - suffix_len, MAYBE_NULL_SUFFIX);
>> +}
>> +
>> +/* Return the type info of a stub function, if it exists.
>> + *
>> + * The name of the stub function is made up of the name of the
>> struct_ops
>> + * and the name of the function pointer member, separated by "__". For
>> + * example, if the struct_ops is named "foo_ops" and the function
>> pointer
>> + * member is named "bar", the stub function name would be
>> "foo_ops__bar".
>> + */
>> +static const struct btf_type *
>> +find_stub_func_proto(struct btf *btf, const char *st_op_name,
>> + const char *member_name)
>> +{
>> + char stub_func_name[MAX_STUB_NAME];
>> + const struct btf_type *t, *func_proto;
>> + s32 btf_id;
>> +
>> + snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s",
>> + st_op_name, member_name);
>> + btf_id = btf_find_by_name_kind(btf, stub_func_name, BTF_KIND_FUNC);
>> + if (btf_id < 0)
>> + return NULL;
>> + t = btf_type_by_id(btf, btf_id);
>> + if (!t)
>> + return NULL;
>> + func_proto = btf_type_by_id(btf, t->type);
>> +
>> + return func_proto;
>> +}
>> +
>> +/* Prepare argument info for every nullable argument of a member of a
>> + * struct_ops type.
>> + *
>> + * Create and initialize a list of struct bpf_struct_ops_member_arg_info
>> + * according to type info of the arguments of the stub functions. (Check
>> + * kCFI for more information about stub functions.)
>> + *
>> + * Each member in the struct_ops type has a struct
>> + * bpf_struct_ops_member_arg_info to provide an array of struct
>> + * bpf_ctx_arg_aux, which in turn provides the information that used
>> by the
>> + * verifier to check the arguments of the BPF struct_ops program
>> assigned
>> + * to the member. Here, we only care about the arguments that are
>> marked as
>> + * __nullable.
>> + *
>> + * The array of struct bpf_ctx_arg_aux is eventually assigned to
>> + * prog->aux->ctx_arg_info of BPF struct_ops programs and passed to the
>> + * verifier. (See check_struct_ops_btf_id())
>> + */
>> +static int prepare_arg_info(struct btf *btf,
>> + const char *st_ops_name,
>> + const char *member_name,
>> + struct bpf_struct_ops_member_arg_info *member_arg_info)
>> +{
>> + const struct btf_type *stub_func_proto, *ptr_type;
>> + struct bpf_ctx_arg_aux *arg_info, *ai_buf = NULL;
>> + const struct btf_param *args;
>> + u32 nargs, arg_no = 0;
>> + const char *arg_name;
>> + s32 arg_btf_id;
>> +
>> + stub_func_proto = find_stub_func_proto(btf, st_ops_name,
>> member_name);
>> + if (!stub_func_proto)
>> + return 0;
>> +
>> + nargs = btf_type_vlen(stub_func_proto);
>> + if (nargs > MAX_BPF_FUNC_REG_ARGS) {
>
> Checking MAX_BPF_FUNC_REG_ARGS on the stub_func_proto may not be the
> right check. It should have been done on the origin func_proto (i.e.
> non-stub) when preparing the func_model in btf_distill_func_proto().
> Please double check.
Got it!
>
> If it needs to do sanity check on nargs of stub_func_proto, a better
> check is to ensure the narg of the stub_func_proto is the same as the
> orig_func_proto instead. This discrepancy probably should have been
> complained by the compiler already but does not harm to check (==) here
> in case the argument type is changed and a force cast is used (more below).
Yes, it should be complained by the compiler. However, we are not sure
if the stub function found is the one assign to .cfi_stubs, or a random
function happening to have a matched name.
>
>> + pr_warn("Cannot support #%u args in stub func %s_stub_%s\n",
>> + nargs, st_ops_name, member_name);
>> + return -EINVAL;
>> + }
>> +
>> + ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
>> + if (!ai_buf)
>> + return -ENOMEM;
>> +
>> + args = btf_params(stub_func_proto);
>> + for (arg_no = 0; arg_no < nargs; arg_no++) {
>> + /* Skip arguments that is not suffixed with
>> + * "__nullable".
>> + */
>> + arg_name = btf_name_by_offset(btf,
>> + args[arg_no].name_off);
>> + if (!match_nullable_suffix(arg_name))
>
> I have a question/request.
>
> On top of tagging nullable, can we extend the ctx_arg_info idea here to
> allow changing the pointer type?
>
> In particular, take a stub function in bpf_tcp_ca.c:
>
> static u32 bpf_tcp_ca_ssthresh(struct tcp_sock *tp)
> {
> return 0;
> }
>
> Instead of the "struct sock *sk" argument as defined in the
> tcp_congestion_ops, the stub function uses "struct tcp_sock *tp'. If we
> can reuse the ctx_arg_info idea here, then it can remove the existing
> way of changing the pointer type from bpf_tcp_ca_is_valid_access.
Yes, it can be. We need a way to annotate the argument we want to
override/promote its type, or generate ctx_arg_info for each
argument of a stub function.
>
>> + continue;
>> +
>> + /* Should be a pointer to struct, array, scalar, or enum */
>> + ptr_type = btf_type_resolve_ptr(btf, args[arg_no].type,
>> + &arg_btf_id);
>> + if (!ptr_type ||
>> + (!btf_type_is_struct(ptr_type) &&
>> + !btf_type_is_array(ptr_type) &&
>> + !btf_type_is_scalar(ptr_type) &&
>> + !btf_is_any_enum(ptr_type))) {
>> + kfree(ai_buf);
>> + return -EINVAL;
>> + }
>> +
>> + /* Fill the information of the new argument */
>> + arg_info = ai_buf + member_arg_info->arg_info_cnt++;
>> + arg_info->reg_type =
>> + PTR_TRUSTED | PTR_MAYBE_NULL | PTR_TO_BTF_ID;
>> + arg_info->btf_id = arg_btf_id;
>> + arg_info->btf = btf;
>> + arg_info->offset = arg_no * sizeof(u64);
>
> I think for the current struct_ops users should be fine to assume
> sizeof(u64). The current struct_ops users should only have
> pointer/scalar argument (meaning there is no struct passed-by-value
> argument).
>
> I still think it is better to get it correct for all trampoline
> supported argument here. Take a look at 720e6a435194 ("bpf: Allow struct
> argument in trampoline based programs") and get_ctx_arg_idx(). It may be
I will add another function to translate arg_no to offset.
> easier (not sure if it is cleaner) to directly store the arg_no into
> arg_info here but arg_info only has offset now. Please think about what
> could be a cleaner way to do it.
The offset here is an offset from the start of a context where
the argument is. The BPF opcode access an argument with it's offset, so
we eventually need to translate the arg_no into the offset. The
difference is translating here or in btf_ctx_access().
The question here is "what is OFFSET for?" Without explanation, it
is hard for people to tell what it is. Maybe, we need to change the its
to ctx_offset or alike.
>
>> + }
>> +
>> + if (!member_arg_info->arg_info_cnt)
>> + kfree(ai_buf);
>> + else
>> + member_arg_info->arg_info = ai_buf;
>> +
>> + return 0;
>> +}
>> +
>> int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>> struct btf *btf,
>> struct bpf_verifier_log *log)
>> {
>> + struct bpf_struct_ops_member_arg_info *member_arg_info;
>> struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
>> const struct btf_member *member;
>> const struct btf_type *t;
>> s32 type_id, value_id;
>> char value_name[128];
>> const char *mname;
>> - int i;
>> + int i, err;
>> if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
>> sizeof(value_name)) {
>> @@ -160,6 +291,11 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> if (!is_valid_value_type(btf, value_id, t, value_name))
>> return -EINVAL;
>> + member_arg_info = kcalloc(btf_type_vlen(t),
>> sizeof(*member_arg_info),
>> + GFP_KERNEL);
>> + if (!member_arg_info)
>> + return -ENOMEM;
>> +
>> for_each_member(i, t, member) {
>> const struct btf_type *func_proto;
>> @@ -167,13 +303,15 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> if (!*mname) {
>> pr_warn("anon member in struct %s is not supported\n",
>> st_ops->name);
>> - return -EOPNOTSUPP;
>> + err = -EOPNOTSUPP;
>> + goto errout;
>> }
>> if (__btf_member_bitfield_size(t, member)) {
>> pr_warn("bit field member %s in struct %s is not
>> supported\n",
>> mname, st_ops->name);
>> - return -EOPNOTSUPP;
>> + err = -EOPNOTSUPP;
>> + goto errout;
>> }
>> func_proto = btf_type_resolve_func_ptr(btf,
>> @@ -185,14 +323,24 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> &st_ops->func_models[i])) {
>> pr_warn("Error in parsing func ptr %s in struct %s\n",
>> mname, st_ops->name);
>> - return -EINVAL;
>> + err = -EINVAL;
>> + goto errout;
>> }
>> +
>> + err = prepare_arg_info(btf, st_ops->name, mname,
>> + member_arg_info + i);
>> + if (err)
>> + goto errout;
>> }
>> + st_ops_desc->member_arg_info = member_arg_info;
>> + st_ops_desc->member_arg_info_cnt = btf_type_vlen(t);
>
> It should be the same as btf_type_vlen(st_ops_desc->type). I would avoid
> this duplicated info within the same st_ops_desc.
Will remove it.
>
>> +
>> if (st_ops->init(btf)) {
>> pr_warn("Error in init bpf_struct_ops %s\n",
>> st_ops->name);
>> - return -EINVAL;
>> + err = -EINVAL;
>> + goto errout;
>> }
>> st_ops_desc->type_id = type_id;
>> @@ -201,6 +349,14 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>> return 0;
>> +
>> +errout:
>> + while (i > 0)
>> + kfree(member_arg_info[--i].arg_info);
>> + kfree(member_arg_info);
>> + st_ops_desc->member_arg_info = NULL;
>> +
>> + return err;
>> }
>> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void
>> *key,
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 20d2160b3db5..fd192f69eb78 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -1699,6 +1699,20 @@ static void btf_free_struct_meta_tab(struct btf
>> *btf)
>> static void btf_free_struct_ops_tab(struct btf *btf)
>> {
>> struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
>> + struct bpf_struct_ops_member_arg_info *ma_info;
>> + int i, j;
>> + u32 cnt;
>> +
>> + if (tab)
>> + for (i = 0; i < tab->cnt; i++) {
>> + ma_info = tab->ops[i].member_arg_info;
>> + if (ma_info) {
>> + cnt = tab->ops[i].member_arg_info_cnt;
>> + for (j = 0; j < cnt; j++)
>> + kfree(ma_info[j].arg_info);
>> + }
>> + kfree(ma_info);
>> + }
>> kfree(tab);
>> btf->struct_ops_tab = NULL;
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index cd4d780e5400..d1d1c2836bc2 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20373,6 +20373,12 @@ static int check_struct_ops_btf_id(struct
>> bpf_verifier_env *env)
>> }
>> }
>> + /* btf_ctx_access() used this to provide argument type info */
>> + prog->aux->ctx_arg_info =
>> + st_ops_desc->member_arg_info[member_idx].arg_info;
>> + prog->aux->ctx_arg_info_size =
>> + st_ops_desc->member_arg_info[member_idx].arg_info_cnt;
>> +
>> prog->aux->attach_func_proto = func_proto;
>> prog->aux->attach_func_name = mname;
>> env->ops = st_ops->verifier_ops;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments.
2024-02-03 1:57 ` Kui-Feng Lee
@ 2024-02-04 0:21 ` Kui-Feng Lee
0 siblings, 0 replies; 15+ messages in thread
From: Kui-Feng Lee @ 2024-02-04 0:21 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/2/24 17:57, Kui-Feng Lee wrote:
>
>
> On 2/2/24 16:40, Martin KaFai Lau wrote:
>> On 2/2/24 2:05 PM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>>
>>> Collect argument information from the type information of stub
>>> functions to
>>> mark arguments of BPF struct_ops programs with PTR_MAYBE_NULL if they
>>> are
>>> nullable. A nullable argument is annotated by suffixing "__nullable" at
>>> the argument name of stub function.
>>>
>>> For nullable arguments, this patch sets an arg_info to label their
>>> reg_type
>>> with PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This makes the
>>> verifier
>>> to check programs and ensure that they properly check the pointer. The
>>> programs should check if the pointer is null before accessing the
>>> pointed
>>> memory.
>>>
>>> The implementer of a struct_ops type should annotate the arguments
>>> that can
>>> be null. The implementer should define a stub function (empty) as a
>>> placeholder for each defined operator. The name of a stub function
>>> should
>>> be in the pattern "<st_op_type>__<operator name>". For example, for
>>> test_maybe_null of struct bpf_testmod_ops, it's stub function name
>>> should
>>> be "bpf_testmod_ops__test_maybe_null". You mark an argument nullable by
>>> suffixing the argument name with "__nullable" at the stub function.
>>>
>>> Since we already has stub functions for kCFI, we just reuse these stub
>>> functions with the naming convention mentioned earlier. These stub
>>> functions with the naming convention is only required if there are
>>> nullable
>>> arguments to annotate. For functions having not nullable arguments, stub
>>> functions are not necessary for the purpose of this patch.
>>>
>>> This patch will prepare a list of struct bpf_ctx_arg_aux, aka
>>> arg_info, for
>>> each member field of a struct_ops type. "arg_info" will be assigned to
>>> "prog->aux->ctx_arg_info" of BPF struct_ops programs in
>>> check_struct_ops_btf_id() so that it can be used by btf_ctx_access()
>>> later
>>> to set reg_type properly for the verifier.
>>
>> I looked at the high level. Some comments below.
>>
>>>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>> include/linux/bpf.h | 17 ++++
>>> kernel/bpf/bpf_struct_ops.c | 166 ++++++++++++++++++++++++++++++++++--
>>> kernel/bpf/btf.c | 14 +++
>>> kernel/bpf/verifier.c | 6 ++
>>> 4 files changed, 198 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index 9a2ee9456989..63ef5cbfd213 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -1709,6 +1709,19 @@ struct bpf_struct_ops {
>>> struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
>>> };
>>> +/* Every member of a struct_ops type has an instance even the member
>>> is not
>>> + * an operator (function pointer). The "arg_info" field will be
>>> assigned to
>>> + * prog->aux->arg_info of BPF struct_ops programs to provide the
>>> argument
>>> + * information required by the verifier to verify the program.
>>> + *
>>> + * btf_ctx_access() will lookup prog->aux->arg_info to find the
>>> + * corresponding entry for an given argument.
>>> + */
>>> +struct bpf_struct_ops_member_arg_info {
>>> + struct bpf_ctx_arg_aux *arg_info;
>>> + u32 arg_info_cnt;
>>> +};
>>> +
>>> struct bpf_struct_ops_desc {
>>> struct bpf_struct_ops *st_ops;
>>> @@ -1716,6 +1729,10 @@ struct bpf_struct_ops_desc {
>>> const struct btf_type *value_type;
>>> u32 type_id;
>>> u32 value_id;
>>> +
>>> + /* Collection of argument information for each member */
>>> + struct bpf_struct_ops_member_arg_info *member_arg_info;
>>> + u32 member_arg_info_cnt;
>>> };
>>> enum bpf_struct_ops_state {
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index f98f580de77a..313f6ceabcf4 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -116,17 +116,148 @@ static bool is_valid_value_type(struct btf
>>> *btf, s32 value_id,
>>> return true;
>>> }
>>> +#define MAYBE_NULL_SUFFIX "__nullable"
>>> +#define MAX_STUB_NAME 128
>>> +
>>> +static int match_nullable_suffix(const char *name)
>>> +{
>>> + int suffix_len, len;
>>> +
>>> + if (!name)
>>> + return 0;
>>> +
>>> + suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
>>> + len = strlen(name);
>>> + if (len < suffix_len)
>>> + return 0;
>>> +
>>> + return !strcmp(name + len - suffix_len, MAYBE_NULL_SUFFIX);
>>> +}
>>> +
>>> +/* Return the type info of a stub function, if it exists.
>>> + *
>>> + * The name of the stub function is made up of the name of the
>>> struct_ops
>>> + * and the name of the function pointer member, separated by "__". For
>>> + * example, if the struct_ops is named "foo_ops" and the function
>>> pointer
>>> + * member is named "bar", the stub function name would be
>>> "foo_ops__bar".
>>> + */
>>> +static const struct btf_type *
>>> +find_stub_func_proto(struct btf *btf, const char *st_op_name,
>>> + const char *member_name)
>>> +{
>>> + char stub_func_name[MAX_STUB_NAME];
>>> + const struct btf_type *t, *func_proto;
>>> + s32 btf_id;
>>> +
>>> + snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s",
>>> + st_op_name, member_name);
>>> + btf_id = btf_find_by_name_kind(btf, stub_func_name, BTF_KIND_FUNC);
>>> + if (btf_id < 0)
>>> + return NULL;
>>> + t = btf_type_by_id(btf, btf_id);
>>> + if (!t)
>>> + return NULL;
>>> + func_proto = btf_type_by_id(btf, t->type);
>>> +
>>> + return func_proto;
>>> +}
>>> +
>>> +/* Prepare argument info for every nullable argument of a member of a
>>> + * struct_ops type.
>>> + *
>>> + * Create and initialize a list of struct
>>> bpf_struct_ops_member_arg_info
>>> + * according to type info of the arguments of the stub functions.
>>> (Check
>>> + * kCFI for more information about stub functions.)
>>> + *
>>> + * Each member in the struct_ops type has a struct
>>> + * bpf_struct_ops_member_arg_info to provide an array of struct
>>> + * bpf_ctx_arg_aux, which in turn provides the information that used
>>> by the
>>> + * verifier to check the arguments of the BPF struct_ops program
>>> assigned
>>> + * to the member. Here, we only care about the arguments that are
>>> marked as
>>> + * __nullable.
>>> + *
>>> + * The array of struct bpf_ctx_arg_aux is eventually assigned to
>>> + * prog->aux->ctx_arg_info of BPF struct_ops programs and passed to the
>>> + * verifier. (See check_struct_ops_btf_id())
>>> + */
>>> +static int prepare_arg_info(struct btf *btf,
>>> + const char *st_ops_name,
>>> + const char *member_name,
>>> + struct bpf_struct_ops_member_arg_info *member_arg_info)
>>> +{
>>> + const struct btf_type *stub_func_proto, *ptr_type;
>>> + struct bpf_ctx_arg_aux *arg_info, *ai_buf = NULL;
>>> + const struct btf_param *args;
>>> + u32 nargs, arg_no = 0;
>>> + const char *arg_name;
>>> + s32 arg_btf_id;
>>> +
>>> + stub_func_proto = find_stub_func_proto(btf, st_ops_name,
>>> member_name);
>>> + if (!stub_func_proto)
>>> + return 0;
>>> +
>>> + nargs = btf_type_vlen(stub_func_proto);
>>> + if (nargs > MAX_BPF_FUNC_REG_ARGS) {
>>
>> Checking MAX_BPF_FUNC_REG_ARGS on the stub_func_proto may not be the
>> right check. It should have been done on the origin func_proto (i.e.
>> non-stub) when preparing the func_model in btf_distill_func_proto().
>> Please double check.
>
> Got it!
>
>>
>> If it needs to do sanity check on nargs of stub_func_proto, a better
>> check is to ensure the narg of the stub_func_proto is the same as the
>> orig_func_proto instead. This discrepancy probably should have been
>> complained by the compiler already but does not harm to check (==)
>> here in case the argument type is changed and a force cast is used
>> (more below).
>
> Yes, it should be complained by the compiler. However, we are not sure
> if the stub function found is the one assign to .cfi_stubs, or a random
> function happening to have a matched name.
>
>>
>>> + pr_warn("Cannot support #%u args in stub func %s_stub_%s\n",
>>> + nargs, st_ops_name, member_name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
>>> + if (!ai_buf)
>>> + return -ENOMEM;
>>> +
>>> + args = btf_params(stub_func_proto);
>>> + for (arg_no = 0; arg_no < nargs; arg_no++) {
>>> + /* Skip arguments that is not suffixed with
>>> + * "__nullable".
>>> + */
>>> + arg_name = btf_name_by_offset(btf,
>>> + args[arg_no].name_off);
>>> + if (!match_nullable_suffix(arg_name))
>>
>> I have a question/request.
>>
>> On top of tagging nullable, can we extend the ctx_arg_info idea here
>> to allow changing the pointer type?
>>
>> In particular, take a stub function in bpf_tcp_ca.c:
>>
>> static u32 bpf_tcp_ca_ssthresh(struct tcp_sock *tp)
>> {
>> return 0;
>> }
>>
>> Instead of the "struct sock *sk" argument as defined in the
>> tcp_congestion_ops, the stub function uses "struct tcp_sock *tp'. If
>> we can reuse the ctx_arg_info idea here, then it can remove the
>> existing way of changing the pointer type from
>> bpf_tcp_ca_is_valid_access.
>
> Yes, it can be. We need a way to annotate the argument we want to
> override/promote its type, or generate ctx_arg_info for each
> argument of a stub function.
By the way, should I support it with the "__override"/"__promote" suffix
or any better one?
>
>>
>>> + continue;
>>> +
>>> + /* Should be a pointer to struct, array, scalar, or enum */
>>> + ptr_type = btf_type_resolve_ptr(btf, args[arg_no].type,
>>> + &arg_btf_id);
>>> + if (!ptr_type ||
>>> + (!btf_type_is_struct(ptr_type) &&
>>> + !btf_type_is_array(ptr_type) &&
>>> + !btf_type_is_scalar(ptr_type) &&
>>> + !btf_is_any_enum(ptr_type))) {
>>> + kfree(ai_buf);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Fill the information of the new argument */
>>> + arg_info = ai_buf + member_arg_info->arg_info_cnt++;
>>> + arg_info->reg_type =
>>> + PTR_TRUSTED | PTR_MAYBE_NULL | PTR_TO_BTF_ID;
>>> + arg_info->btf_id = arg_btf_id;
>>> + arg_info->btf = btf;
>>> + arg_info->offset = arg_no * sizeof(u64);
>>
>> I think for the current struct_ops users should be fine to assume
>> sizeof(u64). The current struct_ops users should only have
>> pointer/scalar argument (meaning there is no struct passed-by-value
>> argument).
>>
>> I still think it is better to get it correct for all trampoline
>> supported argument here. Take a look at 720e6a435194 ("bpf: Allow
>> struct argument in trampoline based programs") and get_ctx_arg_idx().
>> It may be
>
> I will add another function to translate arg_no to offset.
>
>> easier (not sure if it is cleaner) to directly store the arg_no into
>> arg_info here but arg_info only has offset now. Please think about
>> what could be a cleaner way to do it.
>
> The offset here is an offset from the start of a context where
> the argument is. The BPF opcode access an argument with it's offset, so
> we eventually need to translate the arg_no into the offset. The
> difference is translating here or in btf_ctx_access().
>
> The question here is "what is OFFSET for?" Without explanation, it
> is hard for people to tell what it is. Maybe, we need to change the its
> to ctx_offset or alike.
>
>>
>>> + }
>>> +
>>> + if (!member_arg_info->arg_info_cnt)
>>> + kfree(ai_buf);
>>> + else
>>> + member_arg_info->arg_info = ai_buf;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>>> struct btf *btf,
>>> struct bpf_verifier_log *log)
>>> {
>>> + struct bpf_struct_ops_member_arg_info *member_arg_info;
>>> struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
>>> const struct btf_member *member;
>>> const struct btf_type *t;
>>> s32 type_id, value_id;
>>> char value_name[128];
>>> const char *mname;
>>> - int i;
>>> + int i, err;
>>> if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
>>> sizeof(value_name)) {
>>> @@ -160,6 +291,11 @@ int bpf_struct_ops_desc_init(struct
>>> bpf_struct_ops_desc *st_ops_desc,
>>> if (!is_valid_value_type(btf, value_id, t, value_name))
>>> return -EINVAL;
>>> + member_arg_info = kcalloc(btf_type_vlen(t),
>>> sizeof(*member_arg_info),
>>> + GFP_KERNEL);
>>> + if (!member_arg_info)
>>> + return -ENOMEM;
>>> +
>>> for_each_member(i, t, member) {
>>> const struct btf_type *func_proto;
>>> @@ -167,13 +303,15 @@ int bpf_struct_ops_desc_init(struct
>>> bpf_struct_ops_desc *st_ops_desc,
>>> if (!*mname) {
>>> pr_warn("anon member in struct %s is not supported\n",
>>> st_ops->name);
>>> - return -EOPNOTSUPP;
>>> + err = -EOPNOTSUPP;
>>> + goto errout;
>>> }
>>> if (__btf_member_bitfield_size(t, member)) {
>>> pr_warn("bit field member %s in struct %s is not
>>> supported\n",
>>> mname, st_ops->name);
>>> - return -EOPNOTSUPP;
>>> + err = -EOPNOTSUPP;
>>> + goto errout;
>>> }
>>> func_proto = btf_type_resolve_func_ptr(btf,
>>> @@ -185,14 +323,24 @@ int bpf_struct_ops_desc_init(struct
>>> bpf_struct_ops_desc *st_ops_desc,
>>> &st_ops->func_models[i])) {
>>> pr_warn("Error in parsing func ptr %s in struct %s\n",
>>> mname, st_ops->name);
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>> + goto errout;
>>> }
>>> +
>>> + err = prepare_arg_info(btf, st_ops->name, mname,
>>> + member_arg_info + i);
>>> + if (err)
>>> + goto errout;
>>> }
>>> + st_ops_desc->member_arg_info = member_arg_info;
>>> + st_ops_desc->member_arg_info_cnt = btf_type_vlen(t);
>>
>> It should be the same as btf_type_vlen(st_ops_desc->type). I would
>> avoid this duplicated info within the same st_ops_desc.
>
> Will remove it.
>
>>
>>> +
>>> if (st_ops->init(btf)) {
>>> pr_warn("Error in init bpf_struct_ops %s\n",
>>> st_ops->name);
>>> - return -EINVAL;
>>> + err = -EINVAL;
>>> + goto errout;
>>> }
>>> st_ops_desc->type_id = type_id;
>>> @@ -201,6 +349,14 @@ int bpf_struct_ops_desc_init(struct
>>> bpf_struct_ops_desc *st_ops_desc,
>>> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>>> return 0;
>>> +
>>> +errout:
>>> + while (i > 0)
>>> + kfree(member_arg_info[--i].arg_info);
>>> + kfree(member_arg_info);
>>> + st_ops_desc->member_arg_info = NULL;
>>> +
>>> + return err;
>>> }
>>> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map,
>>> void *key,
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 20d2160b3db5..fd192f69eb78 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -1699,6 +1699,20 @@ static void btf_free_struct_meta_tab(struct
>>> btf *btf)
>>> static void btf_free_struct_ops_tab(struct btf *btf)
>>> {
>>> struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
>>> + struct bpf_struct_ops_member_arg_info *ma_info;
>>> + int i, j;
>>> + u32 cnt;
>>> +
>>> + if (tab)
>>> + for (i = 0; i < tab->cnt; i++) {
>>> + ma_info = tab->ops[i].member_arg_info;
>>> + if (ma_info) {
>>> + cnt = tab->ops[i].member_arg_info_cnt;
>>> + for (j = 0; j < cnt; j++)
>>> + kfree(ma_info[j].arg_info);
>>> + }
>>> + kfree(ma_info);
>>> + }
>>> kfree(tab);
>>> btf->struct_ops_tab = NULL;
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index cd4d780e5400..d1d1c2836bc2 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -20373,6 +20373,12 @@ static int check_struct_ops_btf_id(struct
>>> bpf_verifier_env *env)
>>> }
>>> }
>>> + /* btf_ctx_access() used this to provide argument type info */
>>> + prog->aux->ctx_arg_info =
>>> + st_ops_desc->member_arg_info[member_idx].arg_info;
>>> + prog->aux->ctx_arg_info_size =
>>> + st_ops_desc->member_arg_info[member_idx].arg_info_cnt;
>>> +
>>> prog->aux->attach_func_proto = func_proto;
>>> prog->aux->attach_func_name = mname;
>>> env->ops = st_ops->verifier_ops;
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments.
2024-02-03 0:40 ` Martin KaFai Lau
2024-02-03 1:57 ` Kui-Feng Lee
@ 2024-02-05 1:53 ` Kui-Feng Lee
1 sibling, 0 replies; 15+ messages in thread
From: Kui-Feng Lee @ 2024-02-05 1:53 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/2/24 16:40, Martin KaFai Lau wrote:
>
> I have a question/request.
>
> On top of tagging nullable, can we extend the ctx_arg_info idea here to
> allow changing the pointer type?
>
> In particular, take a stub function in bpf_tcp_ca.c:
>
> static u32 bpf_tcp_ca_ssthresh(struct tcp_sock *tp)
> {
> return 0;
> }
>
> Instead of the "struct sock *sk" argument as defined in the
> tcp_congestion_ops, the stub function uses "struct tcp_sock *tp'. If we
> can reuse the ctx_arg_info idea here, then it can remove the existing
> way of changing the pointer type from bpf_tcp_ca_is_valid_access.
>
A question just come to me. Why doesn't just define the argument as a
pointer to struct tpc_sock in the definition of the function pointer?
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-02-05 1:53 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-02 22:05 [RFC bpf-next v4 0/6] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 1/6] bpf: Allow PTR_TO_BTF_ID even for pointers to int thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 2/6] bpf: Extend PTR_TO_BTF_ID to handle pointers to scalar and array types thinker.li
2024-02-03 0:52 ` Martin KaFai Lau
2024-02-03 1:03 ` Kui-Feng Lee
2024-02-02 22:05 ` [RFC bpf-next v4 3/6] bpf: Remove an unnecessary check thinker.li
2024-02-03 0:46 ` Martin KaFai Lau
2024-02-03 1:03 ` Kui-Feng Lee
2024-02-02 22:05 ` [RFC bpf-next v4 4/6] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
2024-02-02 22:05 ` [RFC bpf-next v4 5/6] bpf: Create argument information for nullable arguments thinker.li
2024-02-03 0:40 ` Martin KaFai Lau
2024-02-03 1:57 ` Kui-Feng Lee
2024-02-04 0:21 ` Kui-Feng Lee
2024-02-05 1:53 ` Kui-Feng Lee
2024-02-02 22:05 ` [RFC bpf-next v4 6/6] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox