| Message ID | 20220503171437.666326-3-maximmi@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | New BPF helpers to accelerate synproxy | expand |
On Tue, May 3, 2022 at 10:15 AM Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > > Before this commit, the BPF verifier required ARG_PTR_TO_MEM arguments > to be followed by ARG_CONST_SIZE holding the size of the memory region. > The helpers had to check that size in runtime. > > There are cases where the size expected by a helper is a compile-time > constant. Checking it in runtime is an unnecessary overhead and waste of > BPF registers. > > This commit allows helpers to accept ARG_PTR_TO_MEM arguments without > the corresponding ARG_CONST_SIZE, given that they define the memory > region size in struct bpf_func_proto. I think it's much less confusing and cleaner to have ARG_PTR_TO_FIXED_SIZE_MEM (or whatever similar name), instead of adding special casing to ARG_PTR_TO_MEM. > > Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > --- > include/linux/bpf.h | 10 ++++++++++ > kernel/bpf/verifier.c | 26 +++++++++++++++----------- > 2 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index be94833d390a..255ae3652225 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -514,6 +514,16 @@ struct bpf_func_proto { > }; > u32 *arg_btf_id[5]; > }; > + union { > + struct { > + size_t arg1_size; > + size_t arg2_size; > + size_t arg3_size; > + size_t arg4_size; > + size_t arg5_size; > + }; > + size_t arg_size[5]; > + }; We have almost 250 instances of struct bpf_func_proto variables: $ rg 'const struct bpf_func_proto.* = \{' | wc -l 244 You are adding 40 bytes to it just to use it for 1-2 special prototypes. It is quite expensive, IMHO, to increase vmlinux image size by 10KB just for this. Should we reuse arg_btf_id union (same argument won't be PTR_TO_BTF_ID and PTR_TO_FIXED_SIZE_MEM, right)? Or alternatively special-case those few prototypes in verifier code directly when trying to fetch memory size? > int *ret_btf_id; /* return value btf_id */ > bool (*allowed)(const struct bpf_prog *prog); > }; [...]
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index be94833d390a..255ae3652225 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -514,6 +514,16 @@ struct bpf_func_proto { }; u32 *arg_btf_id[5]; }; + union { + struct { + size_t arg1_size; + size_t arg2_size; + size_t arg3_size; + size_t arg4_size; + size_t arg5_size; + }; + size_t arg_size[5]; + }; int *ret_btf_id; /* return value btf_id */ bool (*allowed)(const struct bpf_prog *prog); }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 813f6ee80419..57fcf2e82f30 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5843,6 +5843,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, * next is_mem_size argument below. */ meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM); + if (fn->arg_size[arg]) { + err = check_helper_mem_access(env, regno, + fn->arg_size[arg], false, + meta); + } } else if (arg_type_is_mem_size(arg_type)) { bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO); @@ -6186,13 +6191,12 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn) return count <= 1; } -static bool check_args_pair_invalid(enum bpf_arg_type arg_curr, - enum bpf_arg_type arg_next) +static bool check_args_pair_invalid(const struct bpf_func_proto *fn, int arg) { - return (arg_type_is_mem_ptr(arg_curr) && - !arg_type_is_mem_size(arg_next)) || - (!arg_type_is_mem_ptr(arg_curr) && - arg_type_is_mem_size(arg_next)); + if (arg_type_is_mem_ptr(fn->arg_type[arg])) + return arg_type_is_mem_size(fn->arg_type[arg + 1]) == + !!fn->arg_size[arg]; + return arg_type_is_mem_size(fn->arg_type[arg + 1]) || fn->arg_size[arg]; } static bool check_arg_pair_ok(const struct bpf_func_proto *fn) @@ -6203,11 +6207,11 @@ static bool check_arg_pair_ok(const struct bpf_func_proto *fn) * helper function specification. */ if (arg_type_is_mem_size(fn->arg1_type) || - arg_type_is_mem_ptr(fn->arg5_type) || - check_args_pair_invalid(fn->arg1_type, fn->arg2_type) || - check_args_pair_invalid(fn->arg2_type, fn->arg3_type) || - check_args_pair_invalid(fn->arg3_type, fn->arg4_type) || - check_args_pair_invalid(fn->arg4_type, fn->arg5_type)) + (arg_type_is_mem_ptr(fn->arg5_type) && !fn->arg5_size) || + check_args_pair_invalid(fn, 0) || + check_args_pair_invalid(fn, 1) || + check_args_pair_invalid(fn, 2) || + check_args_pair_invalid(fn, 3)) return false; return true;