On Fri, Aug 26, 2022 at 3:25 AM Kumar Kartikeya Dwivedi memxor@gmail.com wrote:
On Wed, 24 Aug 2022 at 15:41, Benjamin Tissoires benjamin.tissoires@redhat.com wrote:
For drivers (outside of network), the incoming data is not statically defined in a struct. Most of the time the data buffer is kzalloc-ed and thus we can not rely on eBPF and BTF to explore the data.
This commit allows to return an arbitrary memory, previously allocated by the driver. An interesting extra point is that the kfunc can mark the exported memory region as read only or read/write.
So, when a kfunc is not returning a pointer to a struct but to a plain type, we can consider it is a valid allocated memory assuming that:
- one of the arguments is either called rdonly_buf_size or rdwr_buf_size
- and this argument is a const from the caller point of view
We can then use this parameter as the size of the allocated memory.
The memory is either read-only or read-write based on the name of the size parameter.
Acked-by: Kumar Kartikeya Dwivedi memxor@gmail.com Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com
changes in v9:
- updated to match upstream (replaced kfunc_flag by a field in kfunc_meta)
no changes in v8
changes in v7:
- ensures btf_type_is_struct_ptr() checks for a ptr first (squashed from next commit)
- remove multiple_ref_obj_id need
- use btf_type_skip_modifiers instead of manually doing it in btf_type_is_struct_ptr()
- s/strncmp/strcmp/ in btf_is_kfunc_arg_mem_size()
- check for tnum_is_const when retrieving the size value
- have only one check for "Ensure only one argument is referenced PTR_TO_BTF_ID"
- add some more context to the commit message
changes in v6:
- code review from Kartikeya:
- remove comment change that had no reasons to be
- remove handling of PTR_TO_MEM with kfunc releases
- introduce struct bpf_kfunc_arg_meta
- do rdonly/rdwr_buf_size check in btf_check_kfunc_arg_match
- reverted most of the changes in verifier.c
- make sure kfunc acquire is using a struct pointer, not just a plain pointer
- also forward ref_obj_id to PTR_TO_MEM in kfunc to not use after free the allocated memory
changes in v5:
- updated PTR_TO_MEM comment in btf.c to match upstream
- make it read-only or read-write based on the name of size
new in v4
change btf.h
fix allow kfunc to return an allocated mem
include/linux/bpf.h | 9 +++- include/linux/btf.h | 10 +++++ kernel/bpf/btf.c | 98 ++++++++++++++++++++++++++++++++++--------- kernel/bpf/verifier.c | 43 +++++++++++++------ 4 files changed, 128 insertions(+), 32 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 39bd36359c1e..90dd218e0199 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1932,13 +1932,20 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, const char *func_name, struct btf_func_model *m); [...]
static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, bool ptr_to_mem_ok,
u32 kfunc_flags)
struct bpf_kfunc_arg_meta *kfunc_meta){ enum bpf_prog_type prog_type = resolve_prog_type(env->prog); bool rel = false, kptr_get = false, trusted_arg = false; @@ -6207,12 +6232,12 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return -EINVAL; }
if (is_kfunc) {
if (is_kfunc && kfunc_meta) { /* Only kfunc can be release func */
rel = kfunc_flags & KF_RELEASE;kptr_get = kfunc_flags & KF_KPTR_GET;trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;sleepable = kfunc_flags & KF_SLEEPABLE;
rel = kfunc_meta->flags & KF_RELEASE;kptr_get = kfunc_meta->flags & KF_KPTR_GET;trusted_arg = kfunc_meta->flags & KF_TRUSTED_ARGS;sleepable = kfunc_meta->flags & KF_SLEEPABLE; } /* check that BTF function arguments match actual types that the@@ -6225,6 +6250,35 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
t = btf_type_skip_modifiers(btf, args[i].type, NULL); if (btf_type_is_scalar(t)) {
if (is_kfunc && kfunc_meta) {bool is_buf_size = false;/* check for any const scalar parameter of name "rdonly_buf_size"* or "rdwr_buf_size"*/if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg,"rdonly_buf_size")) {kfunc_meta->r0_rdonly = true;is_buf_size = true;} else if (btf_is_kfunc_arg_mem_size(btf, &args[i], reg,"rdwr_buf_size"))is_buf_size = true;if (is_buf_size) {if (kfunc_meta->r0_size) {bpf_log(log, "2 or more rdonly/rdwr_buf_size parameters for kfunc");return -EINVAL;}if (!tnum_is_const(reg->var_off)) {bpf_log(log, "R%d is not a const\n", regno);return -EINVAL;}kfunc_meta->r0_size = reg->var_off.value;Sorry for not pointing it out before, but you will need a call to mark_chain_precision here after this, since the value of the scalar is being used to decide the size of the returned pointer.
No worries.
I do however have a couple of questions (I have strictly no idea what mark_chain_precision does): - which register number should I call mark_chain_precision() as parameter? r0 or regno (the one with the constant)? - mark_chain_precision() is declared static in verifier.c. Should I export it so btf.c can have access to it, or can I delay the call to mark_chain_precision() in verifier.c when I set regs[BPF_REG_0].mem_size?
}}if (reg->type == SCALAR_VALUE) continue; bpf_log(log, "R%d is not a scalar\n", regno);@@ -6255,6 +6309,19 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, if (ret < 0) return ret;
if (is_kfunc && reg->type == PTR_TO_BTF_ID) {I think you can drop this extra check 'reg->type == PTR_TO_BTF_ID), this condition of only one ref_obj_id should hold regardless of the type.
Ack.
Cheers, Benjamin
[...]