* [PATCH bpf-next v5 0/3] Support PTR_MAYBE_NULL for struct_ops arguments.
@ 2024-02-06 6:38 thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 1/3] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: thinker.li @ 2024-02-06 6:38 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>
Allow passing null pointers to the operators provided by a struct_ops
object. This is an RFC to collect feedbacks/opinions.
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)
{
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 v4:
- Remove the support of pointers to types other than struct
types. That would be a separate patchset.
- Remove the patch about extending PTR_TO_BTF_ID.
- Remove the test against various pointer types from selftests.
- Remove the patch "bpf: Remove an unnecessary check" and send that
patch separately.
- Remove member_arg_info_cnt from struct bpf_struct_ops_desc.
- Use btf_id from FUNC_PROTO of a function pointer instead of a stub
function.
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.
v4: https://lore.kernel.org/all/20240202220516.1165466-1-thinker.li@gmail.com/
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 (3):
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 | 19 ++
kernel/bpf/bpf_struct_ops.c | 185 +++++++++++++++++-
kernel/bpf/btf.c | 42 +++-
kernel/bpf/verifier.c | 6 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 12 +-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 7 +
.../prog_tests/test_struct_ops_maybe_null.c | 47 +++++
.../bpf/progs/struct_ops_maybe_null.c | 31 +++
.../bpf/progs/struct_ops_maybe_null_fail.c | 25 +++
9 files changed, 365 insertions(+), 9 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] 9+ messages in thread
* [PATCH bpf-next v5 1/3] bpf: add btf pointer to struct bpf_ctx_arg_aux.
2024-02-06 6:38 [PATCH bpf-next v5 0/3] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
@ 2024-02-06 6:38 ` thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
2 siblings, 0 replies; 9+ messages in thread
From: thinker.li @ 2024-02-06 6:38 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 f7725cb6e564..aa72674114af 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6266,7 +6266,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] 9+ messages in thread
* [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments.
2024-02-06 6:38 [PATCH bpf-next v5 0/3] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 1/3] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
@ 2024-02-06 6:38 ` thinker.li
2024-02-07 20:45 ` Martin KaFai Lau
2024-02-06 6:38 ` [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
2 siblings, 1 reply; 9+ messages in thread
From: thinker.li @ 2024-02-06 6:38 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 | 18 ++++
kernel/bpf/bpf_struct_ops.c | 185 ++++++++++++++++++++++++++++++++++--
kernel/bpf/btf.c | 40 ++++++++
kernel/bpf/verifier.c | 6 ++
4 files changed, 242 insertions(+), 7 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9a2ee9456989..29d9ec1c4fd9 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,9 @@ 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;
};
enum bpf_struct_ops_state {
@@ -2500,6 +2516,8 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog,
int bpf_prog_test_run_nf(struct bpf_prog *prog,
const union bpf_attr *kattr,
union bpf_attr __user *uattr);
+int btf_ctx_arg_offset(struct btf *btf, const struct btf_type *func_proto,
+ u32 arg_no);
bool btf_ctx_access(int off, int size, enum bpf_access_type type,
const struct bpf_prog *prog,
struct bpf_insn_access_aux *info);
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index f98f580de77a..0db7e12a9244 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -116,17 +116,162 @@ 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 bool match_nullable_suffix(const char *name)
+{
+ int suffix_len, len;
+
+ if (!name)
+ return false;
+
+ suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
+ len = strlen(name);
+ if (len <= suffix_len)
+ return false;
+
+ 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.
+ *
+ * Initialize a struct bpf_struct_ops_member_arg_info according to type
+ * info of the arguments of a stub function. (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())
+ *
+ * member_arg_info->arg_info will be the list of struct bpf_ctx_arg_aux if
+ * success. If fails, it will be kept untouched.
+ */
+static int prepare_arg_info(struct btf *btf,
+ const char *st_ops_name,
+ const char *member_name,
+ const struct btf_type *func_proto,
+ struct bpf_struct_ops_member_arg_info *member_arg_info)
+{
+ const struct btf_type *stub_func_proto, *pointed_type;
+ const struct btf_param *args, *member_args;
+ struct bpf_ctx_arg_aux *arg_info, *ai_buf;
+ u32 nargs, arg_no, arg_info_cnt = 0;
+ const char *arg_name;
+ s32 arg_btf_id;
+ int offset;
+
+ stub_func_proto = find_stub_func_proto(btf, st_ops_name, member_name);
+ if (!stub_func_proto)
+ return 0;
+
+ args = btf_params(stub_func_proto);
+ nargs = btf_type_vlen(stub_func_proto);
+ if (nargs != btf_type_vlen(func_proto)) {
+ pr_warn("the number of arguments of the stub function %s__%s does not match the number of arguments of the member %s of struct %s\n",
+ st_ops_name, member_name, member_name, st_ops_name);
+ return -EINVAL;
+ }
+
+ member_args = btf_params(func_proto);
+
+ ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
+ if (!ai_buf)
+ return -ENOMEM;
+
+ 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 */
+ pointed_type = btf_type_resolve_ptr(btf,
+ member_args[arg_no].type,
+ &arg_btf_id);
+ if (!pointed_type ||
+ !btf_type_is_struct(pointed_type))
+ goto err_out;
+
+ offset = btf_ctx_arg_offset(btf, stub_func_proto, arg_no);
+ if (offset < 0)
+ goto err_out;
+
+ /* Fill the information of the new argument */
+ arg_info = ai_buf + arg_info_cnt++;
+ arg_info->reg_type =
+ PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
+ arg_info->btf_id = arg_btf_id;
+ arg_info->btf = btf;
+ arg_info->offset = offset;
+ }
+
+ if (arg_info_cnt) {
+ member_arg_info->arg_info = ai_buf;
+ member_arg_info->arg_info_cnt = arg_info_cnt;
+ } else {
+ kfree(ai_buf);
+ }
+
+ return 0;
+
+err_out:
+ kfree(ai_buf);
+
+ return -EINVAL;
+}
+
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 +305,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,32 +317,44 @@ 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,
member->type,
NULL);
- if (func_proto &&
- btf_distill_func_proto(log, btf,
+ if (!func_proto)
+ continue;
+
+ if (btf_distill_func_proto(log, btf,
func_proto, mname,
&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,
+ func_proto,
+ member_arg_info + i);
+ if (err)
+ goto errout;
}
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;
@@ -200,7 +362,16 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
st_ops_desc->value_id = value_id;
st_ops_desc->value_type = btf_type_by_id(btf, value_id);
+ st_ops_desc->member_arg_info = member_arg_info;
+
return 0;
+
+errout:
+ while (i > 0)
+ kfree(member_arg_info[--i].arg_info);
+ kfree(member_arg_info);
+
+ 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 aa72674114af..6df390ade2c0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1699,6 +1699,21 @@ 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;
+
+ if (!tab)
+ return;
+
+ for (i = 0; i < tab->cnt; i++) {
+ ma_info = tab->ops[i].member_arg_info;
+ if (!ma_info)
+ continue;
+
+ for (j = 0; j < btf_type_vlen(tab->ops[i].type); j++)
+ kfree(ma_info[j].arg_info);
+ kfree(ma_info);
+ }
kfree(tab);
btf->struct_ops_tab = NULL;
@@ -6130,6 +6145,31 @@ static bool prog_args_trusted(const struct bpf_prog *prog)
}
}
+int btf_ctx_arg_offset(struct btf *btf, const struct btf_type *func_proto,
+ u32 arg_no)
+{
+ const struct btf_param *args;
+ const struct btf_type *t;
+ int off = 0, i;
+ u32 sz, nargs;
+
+ nargs = btf_type_vlen(func_proto);
+ /* It is the return value if arg_no == nargs */
+ if (arg_no > nargs)
+ return -EINVAL;
+
+ args = btf_params(func_proto);
+ for (i = 0; i < arg_no; i++) {
+ t = btf_type_by_id(btf, args[i].type);
+ t = btf_resolve_size(btf, t, &sz);
+ if (IS_ERR(t))
+ return -EINVAL;
+ off += roundup(sz, 8);
+ }
+
+ return off;
+}
+
bool btf_ctx_access(int off, int size, enum bpf_access_type type,
const struct bpf_prog *prog,
struct bpf_insn_access_aux *info)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 64fa188d00ad..8d7e761cda0d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20433,6 +20433,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] 9+ messages in thread
* [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
2024-02-06 6:38 [PATCH bpf-next v5 0/3] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 1/3] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments thinker.li
@ 2024-02-06 6:38 ` thinker.li
2024-02-07 19:35 ` Kui-Feng Lee
2024-02-07 22:38 ` Martin KaFai Lau
2 siblings, 2 replies; 9+ messages in thread
From: thinker.li @ 2024-02-06 6:38 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 | 12 ++++-
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 7 +++
.../prog_tests/test_struct_ops_maybe_null.c | 47 +++++++++++++++++++
.../bpf/progs/struct_ops_maybe_null.c | 31 ++++++++++++
.../bpf/progs/struct_ops_maybe_null_fail.c | 25 ++++++++++
5 files changed, 121 insertions(+), 1 deletion(-)
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 a06daebc75c9..891a2b5f422c 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -555,7 +555,10 @@ static int bpf_dummy_reg(void *kdata)
{
struct bpf_testmod_ops *ops = kdata;
- ops->test_2(4, 3);
+ if (ops->test_maybe_null)
+ ops->test_maybe_null(0, NULL);
+ else
+ ops->test_2(4, 3);
return 0;
}
@@ -573,9 +576,16 @@ static void bpf_testmod_test_2(int a, int b)
{
}
+static int bpf_testmod_ops__test_maybe_null(int dummy,
+ struct task_struct *task__nullable)
+{
+ return 0;
+}
+
static struct bpf_testmod_ops __bpf_testmod_ops = {
.test_1 = bpf_testmod_test_1,
.test_2 = bpf_testmod_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 537beca42896..c51580c9119d 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,14 @@ struct bpf_iter_testmod_seq {
int cnt;
};
+typedef u32 (*ar_t)[2];
+typedef u32 (*ar2_t)[];
+
struct bpf_testmod_ops {
int (*test_1)(void);
void (*test_2)(int a, int b);
+ /* Used to test nullable arguments. */
+ int (*test_maybe_null)(int dummy, struct task_struct *task);
};
#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..1c057c62d893
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
@@ -0,0 +1,47 @@
+// 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 struct_ops_maybe_null_fail *skel;
+
+ skel = struct_ops_maybe_null_fail__open_and_load();
+ if (ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
+ return;
+
+ struct_ops_maybe_null_fail__destroy(skel);
+}
+
+void test_struct_ops_maybe_null(void)
+{
+ /* The verifier verifies the programs at load time, so testing both
+ * programs in the same compile-unit is complicated. We run them in
+ * separate objects to simplify the testing.
+ */
+ 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..c5769c742900
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
@@ -0,0 +1,31 @@
+// 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;
+
+/* 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)
+{
+ if (task)
+ tgid = task->tgid;
+
+ 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..566be47fb40b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
@@ -0,0 +1,25 @@
+// 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;
+
+SEC("struct_ops/test_maybe_null_struct_ptr")
+int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
+ struct task_struct *task)
+{
+ tgid = task->tgid;
+
+ return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_struct_ptr = {
+ .test_maybe_null = (void *)test_maybe_null_struct_ptr,
+};
+
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
2024-02-06 6:38 ` [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
@ 2024-02-07 19:35 ` Kui-Feng Lee
2024-02-07 22:38 ` Martin KaFai Lau
1 sibling, 0 replies; 9+ messages in thread
From: Kui-Feng Lee @ 2024-02-07 19:35 UTC (permalink / raw)
To: thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii,
davemarchevsky, dvernet
Cc: kuifeng
On 2/5/24 22:38, thinker.li@gmail.com wrote:
> 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 | 12 ++++-
> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 7 +++
> .../prog_tests/test_struct_ops_maybe_null.c | 47 +++++++++++++++++++
> .../bpf/progs/struct_ops_maybe_null.c | 31 ++++++++++++
> .../bpf/progs/struct_ops_maybe_null_fail.c | 25 ++++++++++
> 5 files changed, 121 insertions(+), 1 deletion(-)
> 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 a06daebc75c9..891a2b5f422c 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -555,7 +555,10 @@ static int bpf_dummy_reg(void *kdata)
> {
> struct bpf_testmod_ops *ops = kdata;
>
> - ops->test_2(4, 3);
> + if (ops->test_maybe_null)
> + ops->test_maybe_null(0, NULL);
> + else
> + ops->test_2(4, 3);
>
> return 0;
> }
> @@ -573,9 +576,16 @@ static void bpf_testmod_test_2(int a, int b)
> {
> }
>
> +static int bpf_testmod_ops__test_maybe_null(int dummy,
> + struct task_struct *task__nullable)
> +{
> + return 0;
> +}
> +
> static struct bpf_testmod_ops __bpf_testmod_ops = {
> .test_1 = bpf_testmod_test_1,
> .test_2 = bpf_testmod_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 537beca42896..c51580c9119d 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,14 @@ struct bpf_iter_testmod_seq {
> int cnt;
> };
>
> +typedef u32 (*ar_t)[2];
> +typedef u32 (*ar2_t)[];
> +
> struct bpf_testmod_ops {
> int (*test_1)(void);
> void (*test_2)(int a, int b);
> + /* Used to test nullable arguments. */
> + int (*test_maybe_null)(int dummy, struct task_struct *task);
> };
>
> #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..1c057c62d893
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
> @@ -0,0 +1,47 @@
> +// 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 struct_ops_maybe_null_fail *skel;
> +
> + skel = struct_ops_maybe_null_fail__open_and_load();
> + if (ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
> + return;
> +
> + struct_ops_maybe_null_fail__destroy(skel);
> +}
> +
> +void test_struct_ops_maybe_null(void)
> +{
> + /* The verifier verifies the programs at load time, so testing both
> + * programs in the same compile-unit is complicated. We run them in
> + * separate objects to simplify the testing.
> + */
> + 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..c5769c742900
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
> @@ -0,0 +1,31 @@
> +// 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;
> +
> +/* 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.
Just found I didn't remove this comment. I will remove the last two
sentences from the next version.
> + */
> +SEC("struct_ops/test_maybe_null")
> +int BPF_PROG(test_maybe_null, int dummy,
> + struct task_struct *task)
> +{
> + if (task)
> + tgid = task->tgid;
> +
> + 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..566be47fb40b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
> @@ -0,0 +1,25 @@
> +// 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;
> +
> +SEC("struct_ops/test_maybe_null_struct_ptr")
> +int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
> + struct task_struct *task)
> +{
> + tgid = task->tgid;
> +
> + return 0;
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops testmod_struct_ptr = {
> + .test_maybe_null = (void *)test_maybe_null_struct_ptr,
> +};
> +
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments.
2024-02-06 6:38 ` [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments thinker.li
@ 2024-02-07 20:45 ` Martin KaFai Lau
2024-02-07 23:57 ` Kui-Feng Lee
0 siblings, 1 reply; 9+ messages in thread
From: Martin KaFai Lau @ 2024-02-07 20:45 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii,
davemarchevsky, dvernet
On 2/5/24 10:38 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.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 18 ++++
> kernel/bpf/bpf_struct_ops.c | 185 ++++++++++++++++++++++++++++++++++--
> kernel/bpf/btf.c | 40 ++++++++
> kernel/bpf/verifier.c | 6 ++
> 4 files changed, 242 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9a2ee9456989..29d9ec1c4fd9 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 {
nit. Shorten it to bpf_struct_ops_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,9 @@ 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;
nit. Same here on the struct's field name. s/member_arg_info/arg_info(s)/.
> };
>
> enum bpf_struct_ops_state {
> @@ -2500,6 +2516,8 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog,
> int bpf_prog_test_run_nf(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> union bpf_attr __user *uattr);
> +int btf_ctx_arg_offset(struct btf *btf, const struct btf_type *func_proto,
> + u32 arg_no);
This should be in btf.h. Its implementation is also in btf.c in this patch.
> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const struct bpf_prog *prog,
> struct bpf_insn_access_aux *info);
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index f98f580de77a..0db7e12a9244 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -116,17 +116,162 @@ 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 bool match_nullable_suffix(const char *name)
> +{
> + int suffix_len, len;
> +
> + if (!name)
> + return false;
> +
> + suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
> + len = strlen(name);
> + if (len <= suffix_len)
This function looks almost the same as __kfunc_param_match_suffix. How about
refactor __kfunc_param_match_suffix (to btf.c?) and resue here to avoid
duplicating code?
The only difference is between the "<=" here and the "<" there. I think the "<="
here may be a little better.
> + return false;
> +
> + 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 *
I think there is double spaces.
> +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);
check truncated case.
> + 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);
nit. directly "return 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.
> + *
> + * Initialize a struct bpf_struct_ops_member_arg_info according to type
> + * info of the arguments of a stub function. (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())
> + *
> + * member_arg_info->arg_info will be the list of struct bpf_ctx_arg_aux if
> + * success. If fails, it will be kept untouched.
> + */
> +static int prepare_arg_info(struct btf *btf,
> + const char *st_ops_name,
> + const char *member_name,
> + const struct btf_type *func_proto,
> + struct bpf_struct_ops_member_arg_info *member_arg_info)
> +{
> + const struct btf_type *stub_func_proto, *pointed_type;
> + const struct btf_param *args, *member_args;
> + struct bpf_ctx_arg_aux *arg_info, *ai_buf;
> + u32 nargs, arg_no, arg_info_cnt = 0;
> + const char *arg_name;
> + s32 arg_btf_id;
> + int offset;
> +
> + stub_func_proto = find_stub_func_proto(btf, st_ops_name, member_name);
> + if (!stub_func_proto)
> + return 0;
> +
> + args = btf_params(stub_func_proto);
> + nargs = btf_type_vlen(stub_func_proto);
> + if (nargs != btf_type_vlen(func_proto)) {
> + pr_warn("the number of arguments of the stub function %s__%s does not match the number of arguments of the member %s of struct %s\n",
> + st_ops_name, member_name, member_name, st_ops_name);
> + return -EINVAL;
> + }
> +
> + member_args = btf_params(func_proto);
hmm... the "member_args" naming makes me come back here a few times to figure
out "member_args" is the original func_proto or the stub func_proto.
How about using a similar convention like you did for func_proto: "func_proto"
means the original func_proto and "stub_func_proto" means the stub func_proto,
so something like:
"args" means the args of original func_proto and
"stub_args" means the args of the stub_func_proto ?
> +
> + ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
hmm... instead of ai, lets use a fuller name here like arg_info_buf.
> + if (!ai_buf)
> + return -ENOMEM;
> +
> + 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 */
> + pointed_type = btf_type_resolve_ptr(btf,
> + member_args[arg_no].type,
> + &arg_btf_id);
> + if (!pointed_type ||
> + !btf_type_is_struct(pointed_type))
> + goto err_out;
> +
> + offset = btf_ctx_arg_offset(btf, stub_func_proto, arg_no);
The original "func_proto" should be used here.
> + if (offset < 0)
> + goto err_out;
> +
> + /* Fill the information of the new argument */
> + arg_info = ai_buf + arg_info_cnt++;
nit. just init arg_info = arg_info_buf and advance the arg_info++ and
arg_info_cnt++ after the offset assignment below.
> + arg_info->reg_type =
> + PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> + arg_info->btf_id = arg_btf_id;
> + arg_info->btf = btf;
> + arg_info->offset = offset;
> + }
> +
> + if (arg_info_cnt) {
> + member_arg_info->arg_info = ai_buf;
> + member_arg_info->arg_info_cnt = arg_info_cnt;
> + } else {
> + kfree(ai_buf);
> + }
> +
> + return 0;
> +
> +err_out:
> + kfree(ai_buf);
> +
> + return -EINVAL;
> +}
> +
> 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 +305,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,32 +317,44 @@ 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,
> member->type,
> NULL);
> - if (func_proto &&
> - btf_distill_func_proto(log, btf,
> + if (!func_proto)
> + continue;
> +
> + if (btf_distill_func_proto(log, btf,
> func_proto, mname,
> &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,
> + func_proto,
> + member_arg_info + i);
> + if (err)
> + goto errout;
> }
>
> 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;
> @@ -200,7 +362,16 @@ int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> st_ops_desc->value_id = value_id;
> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>
> + st_ops_desc->member_arg_info = member_arg_info;
> +
> return 0;
> +
> +errout:
> + while (i > 0)
> + kfree(member_arg_info[--i].arg_info);
> + kfree(member_arg_info);
> +
> + 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 aa72674114af..6df390ade2c0 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1699,6 +1699,21 @@ 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;
Please try to stay consistent with the variable naming in the same patch. The
other of this patch is using the "member_arg_info" name.
May be just use "arg_infos" everywhere.
> + int i, j;
> +
> + if (!tab)
> + return;
> +
> + for (i = 0; i < tab->cnt; i++) {
> + ma_info = tab->ops[i].member_arg_info;
> + if (!ma_info)
> + continue;
> +
> + for (j = 0; j < btf_type_vlen(tab->ops[i].type); j++)
> + kfree(ma_info[j].arg_info);
> + kfree(ma_info);
Lets refactor this part out to "bpf_struct_ops_desc_release(struct
bpf_struct_ops_desc *)" such that it can be reused with the "errout" path of the
"bpf_struct_ops_desc_init()" above.
> + }
>
> kfree(tab);
> btf->struct_ops_tab = NULL;
> @@ -6130,6 +6145,31 @@ static bool prog_args_trusted(const struct bpf_prog *prog)
> }
> }
>
> +int btf_ctx_arg_offset(struct btf *btf, const struct btf_type *func_proto,
> + u32 arg_no)
> +{
> + const struct btf_param *args;
> + const struct btf_type *t;
> + int off = 0, i;
> + u32 sz, nargs;
> +
> + nargs = btf_type_vlen(func_proto);
> + /* It is the return value if arg_no == nargs */
> + if (arg_no > nargs)
> + return -EINVAL;
> +
> + args = btf_params(func_proto);
> + for (i = 0; i < arg_no; i++) {
> + t = btf_type_by_id(btf, args[i].type);
> + t = btf_resolve_size(btf, t, &sz);
> + if (IS_ERR(t))
> + return -EINVAL;
> + off += roundup(sz, 8);
> + }
> +
> + return off;
> +}
> +
> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const struct bpf_prog *prog,
> struct bpf_insn_access_aux *info)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 64fa188d00ad..8d7e761cda0d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20433,6 +20433,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] 9+ messages in thread
* Re: [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
2024-02-06 6:38 ` [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
2024-02-07 19:35 ` Kui-Feng Lee
@ 2024-02-07 22:38 ` Martin KaFai Lau
2024-02-08 0:54 ` Kui-Feng Lee
1 sibling, 1 reply; 9+ messages in thread
From: Martin KaFai Lau @ 2024-02-07 22:38 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii,
davemarchevsky, dvernet
On 2/5/24 10:38 PM, thinker.li@gmail.com wrote:
> 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.
The commit message needs an update. probably make sense to skip what pointer
type is supported because this patch set does not change that.
>
> 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 | 12 ++++-
> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 7 +++
> .../prog_tests/test_struct_ops_maybe_null.c | 47 +++++++++++++++++++
> .../bpf/progs/struct_ops_maybe_null.c | 31 ++++++++++++
> .../bpf/progs/struct_ops_maybe_null_fail.c | 25 ++++++++++
> 5 files changed, 121 insertions(+), 1 deletion(-)
> 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 a06daebc75c9..891a2b5f422c 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -555,7 +555,10 @@ static int bpf_dummy_reg(void *kdata)
> {
> struct bpf_testmod_ops *ops = kdata;
>
> - ops->test_2(4, 3);
> + if (ops->test_maybe_null)
> + ops->test_maybe_null(0, NULL);
afaict, the "static void maybe_null(void)" test below does not exercise this
line of change.
> + else
> + ops->test_2(4, 3);
>
> return 0;
> }
> @@ -573,9 +576,16 @@ static void bpf_testmod_test_2(int a, int b)
> {
> }
>
> +static int bpf_testmod_ops__test_maybe_null(int dummy,
> + struct task_struct *task__nullable)
> +{
> + return 0;
> +}
> +
> static struct bpf_testmod_ops __bpf_testmod_ops = {
> .test_1 = bpf_testmod_test_1,
> .test_2 = bpf_testmod_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 537beca42896..c51580c9119d 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,14 @@ struct bpf_iter_testmod_seq {
> int cnt;
> };
>
> +typedef u32 (*ar_t)[2];
> +typedef u32 (*ar2_t)[];
They are not needed in v5.
> +
> struct bpf_testmod_ops {
> int (*test_1)(void);
> void (*test_2)(int a, int b);
> + /* Used to test nullable arguments. */
> + int (*test_maybe_null)(int dummy, struct task_struct *task);
> };
>
> #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..1c057c62d893
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +#include <test_progs.h>
> +#include <time.h>
Why 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 struct_ops_maybe_null_fail *skel;
> +
> + skel = struct_ops_maybe_null_fail__open_and_load();
> + if (ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
> + return;
> +
> + struct_ops_maybe_null_fail__destroy(skel);
> +}
> +
> +void test_struct_ops_maybe_null(void)
> +{
> + /* The verifier verifies the programs at load time, so testing both
> + * programs in the same compile-unit is complicated. We run them in
> + * separate objects to simplify the testing.
> + */
> + 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..c5769c742900
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
> @@ -0,0 +1,31 @@
> +// 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;
u64 here.
> +
> +/* 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)
> +{
> + if (task)
> + tgid = task->tgid;
> +
> + 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..566be47fb40b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
> @@ -0,0 +1,25 @@
> +// 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;
but int here.
understand that it does not matter and not the focus of this test but still
better be consistent and use the correct one.
> +
> +SEC("struct_ops/test_maybe_null_struct_ptr")
> +int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
> + struct task_struct *task)
> +{
> + tgid = task->tgid;
> +
> + return 0;
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops testmod_struct_ptr = {
> + .test_maybe_null = (void *)test_maybe_null_struct_ptr,
> +};
> +
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments.
2024-02-07 20:45 ` Martin KaFai Lau
@ 2024-02-07 23:57 ` Kui-Feng Lee
0 siblings, 0 replies; 9+ messages in thread
From: Kui-Feng Lee @ 2024-02-07 23:57 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/7/24 12:45, Martin KaFai Lau wrote:
> On 2/5/24 10:38 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.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 18 ++++
>> kernel/bpf/bpf_struct_ops.c | 185 ++++++++++++++++++++++++++++++++++--
>> kernel/bpf/btf.c | 40 ++++++++
>> kernel/bpf/verifier.c | 6 ++
>> 4 files changed, 242 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 9a2ee9456989..29d9ec1c4fd9 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 {
>
> nit. Shorten it to bpf_struct_ops_arg_info.
Sure!
>
>> + 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,9 @@ 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;
>
> nit. Same here on the struct's field name. s/member_arg_info/arg_info(s)/.
>
Sure!
>> };
>> enum bpf_struct_ops_state {
>> @@ -2500,6 +2516,8 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog
>> *prog,
>> int bpf_prog_test_run_nf(struct bpf_prog *prog,
>> const union bpf_attr *kattr,
>> union bpf_attr __user *uattr);
>> +int btf_ctx_arg_offset(struct btf *btf, const struct btf_type
>> *func_proto,
>> + u32 arg_no);
>
> This should be in btf.h. Its implementation is also in btf.c in this patch.
Got it! I just followed btf_ctx_access().
But, moving to btf.h make sense.
>
>> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> const struct bpf_prog *prog,
>> struct bpf_insn_access_aux *info);
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index f98f580de77a..0db7e12a9244 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -116,17 +116,162 @@ 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 bool match_nullable_suffix(const char *name)
>> +{
>> + int suffix_len, len;
>> +
>> + if (!name)
>> + return false;
>> +
>> + suffix_len = sizeof(MAYBE_NULL_SUFFIX) - 1;
>> + len = strlen(name);
>> + if (len <= suffix_len)
>
> This function looks almost the same as __kfunc_param_match_suffix. How
> about refactor __kfunc_param_match_suffix (to btf.c?) and resue here to
> avoid duplicating code?
Sure!
>
> The only difference is between the "<=" here and the "<" there. I think
> the "<=" here may be a little better.
>
>> + return false;
>> +
>> + 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 *
>
> I think there is double spaces.
>
>> +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);
>
> check truncated case.
Sure!
>
>> + 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);
>
> nit. directly "return 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.
>> + *
>> + * Initialize a struct bpf_struct_ops_member_arg_info according to type
>> + * info of the arguments of a stub function. (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())
>> + *
>> + * member_arg_info->arg_info will be the list of struct
>> bpf_ctx_arg_aux if
>> + * success. If fails, it will be kept untouched.
>> + */
>> +static int prepare_arg_info(struct btf *btf,
>> + const char *st_ops_name,
>> + const char *member_name,
>> + const struct btf_type *func_proto,
>> + struct bpf_struct_ops_member_arg_info *member_arg_info)
>> +{
>> + const struct btf_type *stub_func_proto, *pointed_type;
>> + const struct btf_param *args, *member_args;
>> + struct bpf_ctx_arg_aux *arg_info, *ai_buf;
>> + u32 nargs, arg_no, arg_info_cnt = 0;
>> + const char *arg_name;
>> + s32 arg_btf_id;
>> + int offset;
>> +
>> + stub_func_proto = find_stub_func_proto(btf, st_ops_name,
>> member_name);
>> + if (!stub_func_proto)
>> + return 0;
>> +
>> + args = btf_params(stub_func_proto);
>> + nargs = btf_type_vlen(stub_func_proto);
>> + if (nargs != btf_type_vlen(func_proto)) {
>> + pr_warn("the number of arguments of the stub function %s__%s
>> does not match the number of arguments of the member %s of struct %s\n",
>> + st_ops_name, member_name, member_name, st_ops_name);
>> + return -EINVAL;
>> + }
>> +
>> + member_args = btf_params(func_proto);
>
> hmm... the "member_args" naming makes me come back here a few times to
> figure out "member_args" is the original func_proto or the stub func_proto.
>
> How about using a similar convention like you did for func_proto:
> "func_proto" means the original func_proto and "stub_func_proto" means
> the stub func_proto,
>
> so something like:
>
> "args" means the args of original func_proto and
> "stub_args" means the args of the stub_func_proto ?
ok! I will rename all member_*.
>
>> +
>> + ai_buf = kcalloc(nargs, sizeof(*ai_buf), GFP_KERNEL);
>
> hmm... instead of ai, lets use a fuller name here like arg_info_buf.
>
>> + if (!ai_buf)
>> + return -ENOMEM;
>> +
>> + 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 */
>> + pointed_type = btf_type_resolve_ptr(btf,
>> + member_args[arg_no].type,
>> + &arg_btf_id);
>> + if (!pointed_type ||
>> + !btf_type_is_struct(pointed_type))
>> + goto err_out;
>> +
>> + offset = btf_ctx_arg_offset(btf, stub_func_proto, arg_no);
>
> The original "func_proto" should be used here.
Right!
>
>> + if (offset < 0)
>> + goto err_out;
>> +
>> + /* Fill the information of the new argument */
>> + arg_info = ai_buf + arg_info_cnt++;
>
> nit. just init arg_info = arg_info_buf and advance the arg_info++ and
> arg_info_cnt++ after the offset assignment below.
>
>> + arg_info->reg_type =
>> + PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
>> + arg_info->btf_id = arg_btf_id;
>> + arg_info->btf = btf;
>> + arg_info->offset = offset;
>> + }
>> +
>> + if (arg_info_cnt) {
>> + member_arg_info->arg_info = ai_buf;
>> + member_arg_info->arg_info_cnt = arg_info_cnt;
>> + } else {
>> + kfree(ai_buf);
>> + }
>> +
>> + return 0;
>> +
>> +err_out:
>> + kfree(ai_buf);
>> +
>> + return -EINVAL;
>> +}
>> +
>> 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 +305,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,32 +317,44 @@ 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,
>> member->type,
>> NULL);
>> - if (func_proto &&
>> - btf_distill_func_proto(log, btf,
>> + if (!func_proto)
>> + continue;
>> +
>> + if (btf_distill_func_proto(log, btf,
>> func_proto, mname,
>> &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,
>> + func_proto,
>> + member_arg_info + i);
>> + if (err)
>> + goto errout;
>> }
>> 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;
>> @@ -200,7 +362,16 @@ int bpf_struct_ops_desc_init(struct
>> bpf_struct_ops_desc *st_ops_desc,
>> st_ops_desc->value_id = value_id;
>> st_ops_desc->value_type = btf_type_by_id(btf, value_id);
>> + st_ops_desc->member_arg_info = member_arg_info;
>> +
>> return 0;
>> +
>> +errout:
>> + while (i > 0)
>> + kfree(member_arg_info[--i].arg_info);
>> + kfree(member_arg_info);
>> +
>> + 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 aa72674114af..6df390ade2c0 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -1699,6 +1699,21 @@ 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;
>
> Please try to stay consistent with the variable naming in the same
> patch. The other of this patch is using the "member_arg_info" name.
>
> May be just use "arg_infos" everywhere.
I will rename all member_*.
>
>> + int i, j;
>> +
>> + if (!tab)
>> + return;
>> +
>> + for (i = 0; i < tab->cnt; i++) {
>> + ma_info = tab->ops[i].member_arg_info;
>> + if (!ma_info)
>> + continue;
>> +
>> + for (j = 0; j < btf_type_vlen(tab->ops[i].type); j++)
>> + kfree(ma_info[j].arg_info);
>> + kfree(ma_info);
>
>
> Lets refactor this part out to "bpf_struct_ops_desc_release(struct
> bpf_struct_ops_desc *)" such that it can be reused with the "errout"
> path of the "bpf_struct_ops_desc_init()" above.
Sure!
>
>
>> + }
>> kfree(tab);
>> btf->struct_ops_tab = NULL;
>> @@ -6130,6 +6145,31 @@ static bool prog_args_trusted(const struct
>> bpf_prog *prog)
>> }
>> }
>> +int btf_ctx_arg_offset(struct btf *btf, const struct btf_type
>> *func_proto,
>> + u32 arg_no)
>> +{
>> + const struct btf_param *args;
>> + const struct btf_type *t;
>> + int off = 0, i;
>> + u32 sz, nargs;
>> +
>> + nargs = btf_type_vlen(func_proto);
>> + /* It is the return value if arg_no == nargs */
>> + if (arg_no > nargs)
>> + return -EINVAL;
>> +
>> + args = btf_params(func_proto);
>> + for (i = 0; i < arg_no; i++) {
>> + t = btf_type_by_id(btf, args[i].type);
>> + t = btf_resolve_size(btf, t, &sz);
>> + if (IS_ERR(t))
>> + return -EINVAL;
>> + off += roundup(sz, 8);
>> + }
>> +
>> + return off;
>> +}
>> +
>> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> const struct bpf_prog *prog,
>> struct bpf_insn_access_aux *info)
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 64fa188d00ad..8d7e761cda0d 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20433,6 +20433,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] 9+ messages in thread
* Re: [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators.
2024-02-07 22:38 ` Martin KaFai Lau
@ 2024-02-08 0:54 ` Kui-Feng Lee
0 siblings, 0 replies; 9+ messages in thread
From: Kui-Feng Lee @ 2024-02-08 0:54 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, davemarchevsky,
dvernet
On 2/7/24 14:38, Martin KaFai Lau wrote:
> On 2/5/24 10:38 PM, thinker.li@gmail.com wrote:
>> 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.
>
> The commit message needs an update. probably make sense to skip what
> pointer type is supported because this patch set does not change that.
Agree!
>
>>
>> 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 | 12 ++++-
>> .../selftests/bpf/bpf_testmod/bpf_testmod.h | 7 +++
>> .../prog_tests/test_struct_ops_maybe_null.c | 47 +++++++++++++++++++
>> .../bpf/progs/struct_ops_maybe_null.c | 31 ++++++++++++
>> .../bpf/progs/struct_ops_maybe_null_fail.c | 25 ++++++++++
>> 5 files changed, 121 insertions(+), 1 deletion(-)
>> 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 a06daebc75c9..891a2b5f422c 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -555,7 +555,10 @@ static int bpf_dummy_reg(void *kdata)
>> {
>> struct bpf_testmod_ops *ops = kdata;
>> - ops->test_2(4, 3);
>> + if (ops->test_maybe_null)
>> + ops->test_maybe_null(0, NULL);
>
> afaict, the "static void maybe_null(void)" test below does not exercise
> this line of change.
I will remove it.
>
>> + else
>> + ops->test_2(4, 3);
>> return 0;
>> }
>> @@ -573,9 +576,16 @@ static void bpf_testmod_test_2(int a, int b)
>> {
>> }
>> +static int bpf_testmod_ops__test_maybe_null(int dummy,
>> + struct task_struct *task__nullable)
>> +{
>> + return 0;
>> +}
>> +
>> static struct bpf_testmod_ops __bpf_testmod_ops = {
>> .test_1 = bpf_testmod_test_1,
>> .test_2 = bpf_testmod_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 537beca42896..c51580c9119d 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,14 @@ struct bpf_iter_testmod_seq {
>> int cnt;
>> };
>> +typedef u32 (*ar_t)[2];
>> +typedef u32 (*ar2_t)[];
>
> They are not needed in v5.
Sure!
>
>> +
>> struct bpf_testmod_ops {
>> int (*test_1)(void);
>> void (*test_2)(int a, int b);
>> + /* Used to test nullable arguments. */
>> + int (*test_maybe_null)(int dummy, struct task_struct *task);
>> };
>> #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..1c057c62d893
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +#include <test_progs.h>
>> +#include <time.h>
>
> Why time.h?
It should be removed now.
>
>> +
>> +#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 struct_ops_maybe_null_fail *skel;
>> +
>> + skel = struct_ops_maybe_null_fail__open_and_load();
>> + if (ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
>> + return;
>> +
>> + struct_ops_maybe_null_fail__destroy(skel);
>> +}
>> +
>> +void test_struct_ops_maybe_null(void)
>> +{
>> + /* The verifier verifies the programs at load time, so testing both
>> + * programs in the same compile-unit is complicated. We run them in
>> + * separate objects to simplify the testing.
>> + */
>> + 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..c5769c742900
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
>> @@ -0,0 +1,31 @@
>> +// 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;
>
> u64 here.
>
>> +
>> +/* 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)
>> +{
>> + if (task)
>> + tgid = task->tgid;
>> +
>> + 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..566be47fb40b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
>> @@ -0,0 +1,25 @@
>> +// 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;
>
> but int here.
>
> understand that it does not matter and not the focus of this test but
> still better be consistent and use the correct one.
I will chnage them to pid_t.
>
>> +
>> +SEC("struct_ops/test_maybe_null_struct_ptr")
>> +int BPF_PROG(test_maybe_null_struct_ptr, int dummy,
>> + struct task_struct *task)
>> +{
>> + tgid = task->tgid;
>> +
>> + return 0;
>> +}
>> +
>> +SEC(".struct_ops.link")
>> +struct bpf_testmod_ops testmod_struct_ptr = {
>> + .test_maybe_null = (void *)test_maybe_null_struct_ptr,
>> +};
>> +
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-08 0:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-06 6:38 [PATCH bpf-next v5 0/3] Support PTR_MAYBE_NULL for struct_ops arguments thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 1/3] bpf: add btf pointer to struct bpf_ctx_arg_aux thinker.li
2024-02-06 6:38 ` [PATCH bpf-next v5 2/3] bpf: Create argument information for nullable arguments thinker.li
2024-02-07 20:45 ` Martin KaFai Lau
2024-02-07 23:57 ` Kui-Feng Lee
2024-02-06 6:38 ` [PATCH bpf-next v5 3/3] selftests/bpf: Test PTR_MAYBE_NULL arguments of struct_ops operators thinker.li
2024-02-07 19:35 ` Kui-Feng Lee
2024-02-07 22:38 ` Martin KaFai Lau
2024-02-08 0:54 ` Kui-Feng Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox