| Message ID | 20231023224100.2573116-4-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: File verification with LSM and fsverity | expand |
On Mon, Oct 23, 2023 at 3:41 PM Song Liu <song@kernel.org> wrote: > + > + __bpf_kfunc bpf_get_file_xattr(..., const char *name__const_str, ... > + case KF_ARG_PTR_TO_CONST_STR: CONST_STR was ok here, but as __const_str suffix is a bit too verbose. How about just __str ? I don't think we'll have non-const strings in the near future.
> On Oct 23, 2023, at 5:49 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Oct 23, 2023 at 3:41 PM Song Liu <song@kernel.org> wrote: >> + >> + __bpf_kfunc bpf_get_file_xattr(..., const char *name__const_str, > ... >> + case KF_ARG_PTR_TO_CONST_STR: > > CONST_STR was ok here, but as __const_str suffix is a bit too verbose. > How about just __str ? I don't think we'll have non-const strings in > the near future. I thought about this. While we don't foresee non-const strings in the near future, I think __const_str is acceptable. These annotations are part of the core APIs of kfuncs. As we enabled other subsystems to add kfuncs without touching BPF core, it makes sense to keep the annoations as stable as possible. Making __const_str a little shorter doesn't seem to justify the risk of changing it in the future. Also, we already have longer annotations like __refcounted_kptr. So I personally prefer to keep the annotation as __const_str. Does this make sense? Thanks, Song
On Mon, Oct 23, 2023 at 8:25 PM Song Liu <songliubraving@meta.com> wrote: > > > > > On Oct 23, 2023, at 5:49 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Oct 23, 2023 at 3:41 PM Song Liu <song@kernel.org> wrote: > >> + > >> + __bpf_kfunc bpf_get_file_xattr(..., const char *name__const_str, > > ... > >> + case KF_ARG_PTR_TO_CONST_STR: > > > > CONST_STR was ok here, but as __const_str suffix is a bit too verbose. > > How about just __str ? I don't think we'll have non-const strings in > > the near future. > > I thought about this. While we don't foresee non-const strings in the > near future, I think __const_str is acceptable. These annotations are > part of the core APIs of kfuncs. As we enabled other subsystems to add > kfuncs without touching BPF core, it makes sense to keep the annoations > as stable as possible. Making __const_str a little shorter doesn't seem > to justify the risk of changing it in the future. > > Also, we already have longer annotations like __refcounted_kptr. So I > personally prefer to keep the annotation as __const_str. Ok. That's fair. Didn't realize that such a long suffix is already in use.
diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst index 0d2647fb358d..e696aca08b3a 100644 --- a/Documentation/bpf/kfuncs.rst +++ b/Documentation/bpf/kfuncs.rst @@ -137,6 +137,30 @@ Either way, the returned buffer is either NULL, or of size buffer_szk. Without t annotation, the verifier will reject the program if a null pointer is passed in with a nonzero size. +2.2.5 __const_str Annotation +---------------------------- +This annotation is used to indicate that the argument is a constant string. + +An example is given below:: + + __bpf_kfunc bpf_get_file_xattr(..., const char *name__const_str, ...) + { + ... + } + +In this case, ``bpf_get_file_xattr()`` can be called as:: + + bpf_get_file_xattr(..., "xattr_name", ...); + +Or:: + + const char name[] = "xattr_name"; /* This need to be global */ + int BPF_PROG(...) + { + ... + bpf_get_file_xattr(..., name, ...); + ... + } .. _BPF_kfunc_nodef: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6ce5f0fbad84..a2d992ca49c4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10350,6 +10350,11 @@ static bool is_kfunc_arg_nullable(const struct btf *btf, const struct btf_param return __kfunc_param_match_suffix(btf, arg, "__nullable"); } +static bool is_kfunc_arg_const_str(const struct btf *btf, const struct btf_param *arg) +{ + return __kfunc_param_match_suffix(btf, arg, "__const_str"); +} + static bool is_kfunc_arg_scalar_with_name(const struct btf *btf, const struct btf_param *arg, const char *name) @@ -10493,6 +10498,7 @@ enum kfunc_ptr_arg_type { KF_ARG_PTR_TO_RB_ROOT, KF_ARG_PTR_TO_RB_NODE, KF_ARG_PTR_TO_NULL, + KF_ARG_PTR_TO_CONST_STR, }; enum special_kfunc_type { @@ -10637,6 +10643,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env, if (is_kfunc_arg_rbtree_node(meta->btf, &args[argno])) return KF_ARG_PTR_TO_RB_NODE; + if (is_kfunc_arg_const_str(meta->btf, &args[argno])) + return KF_ARG_PTR_TO_CONST_STR; + if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) { if (!btf_type_is_struct(ref_t)) { verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n", @@ -11260,6 +11269,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ case KF_ARG_PTR_TO_MEM_SIZE: case KF_ARG_PTR_TO_CALLBACK: case KF_ARG_PTR_TO_REFCOUNTED_KPTR: + case KF_ARG_PTR_TO_CONST_STR: /* Trusted by default */ break; default: @@ -11531,6 +11541,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ meta->arg_btf = reg->btf; meta->arg_btf_id = reg->btf_id; break; + case KF_ARG_PTR_TO_CONST_STR: + if (reg->type != PTR_TO_MAP_VALUE) { + verbose(env, "arg#%d doesn't point to a const string\n", i); + return -EINVAL; + } + ret = check_reg_const_str(env, reg, regno); + if (ret) + return ret; + break; } }
KF_ARG_PTR_TO_CONST_STR specifies kfunc args that point to const strings. This is similar to ARG_PTR_TO_CONST_STR for helpers. Signed-off-by: Song Liu <song@kernel.org> --- Documentation/bpf/kfuncs.rst | 24 ++++++++++++++++++++++++ kernel/bpf/verifier.c | 19 +++++++++++++++++++ 2 files changed, 43 insertions(+)