On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@xxxxxx> wrote:
>
> From: Joanne Koong <joannelkoong@xxxxxxxxx>
>
> Instead of having uninitialized versions of arguments as separate
> bpf_arg_types (eg ARG_PTR_TO_UNINIT_MEM as the uninitialized version
> of ARG_PTR_TO_MEM), we can instead use MEM_UNINIT as a bpf_type_flag
> modifier to denote that the argument is uninitialized.
>
> Doing so cleans up some of the logic in the verifier. We no longer
> need to do two checks against an argument type (eg "if
> (base_type(arg_type) == ARG_PTR_TO_MEM || base_type(arg_type) ==
> ARG_PTR_TO_UNINIT_MEM)"), since uninitialized and initialized
> versions of the same argument type will now share the same base type.
>
> In the near future, MEM_UNINIT will be used by dynptr helper functions
> as well.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> include/linux/bpf.h | 19 +++++++++++--------
> kernel/bpf/bpf_lsm.c | 4 ++--
> kernel/bpf/cgroup.c | 4 ++--
> kernel/bpf/helpers.c | 12 ++++++------
> kernel/bpf/stackmap.c | 6 +++---
> kernel/bpf/verifier.c | 25 ++++++++++---------------
> kernel/trace/bpf_trace.c | 20 ++++++++++----------
> net/core/filter.c | 26 +++++++++++++-------------
> 8 files changed, 57 insertions(+), 59 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index bdb5298735ce..6f2558da9d4a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -342,7 +342,9 @@ enum bpf_type_flag {
> */
> MEM_PERCPU = BIT(4 + BPF_BASE_TYPE_BITS),
>
> - __BPF_TYPE_LAST_FLAG = MEM_PERCPU,
> + MEM_UNINIT = BIT(5 + BPF_BASE_TYPE_BITS),
> +
> + __BPF_TYPE_LAST_FLAG = MEM_UNINIT,
> };
>
> /* Max number of base types. */
> @@ -361,16 +363,11 @@ enum bpf_arg_type {
> ARG_CONST_MAP_PTR, /* const argument used as pointer to bpf_map */
> ARG_PTR_TO_MAP_KEY, /* pointer to stack used as map key */
> ARG_PTR_TO_MAP_VALUE, /* pointer to stack used as map value */
> - ARG_PTR_TO_UNINIT_MAP_VALUE, /* pointer to valid memory used to store a map value */
>
> - /* the following constraints used to prototype bpf_memcmp() and other
> - * functions that access data on eBPF program stack
> + /* Used to prototype bpf_memcmp() and other functions that access data
> + * on eBPF program stack
> */
> ARG_PTR_TO_MEM, /* pointer to valid memory (stack, packet, map value) */
> - ARG_PTR_TO_UNINIT_MEM, /* pointer to memory does not need to be initialized,
> - * helper function must fill all bytes or clear
> - * them in error case.
> - */
>
> ARG_CONST_SIZE, /* number of bytes accessed from memory */
> ARG_CONST_SIZE_OR_ZERO, /* number of bytes accessed from memory or 0 */
> @@ -400,6 +397,12 @@ enum bpf_arg_type {
> ARG_PTR_TO_SOCKET_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_SOCKET,
> ARG_PTR_TO_ALLOC_MEM_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_ALLOC_MEM,
> ARG_PTR_TO_STACK_OR_NULL = PTR_MAYBE_NULL | ARG_PTR_TO_STACK,
> + /* pointer to valid memory used to store a map value */
> + ARG_PTR_TO_MAP_VALUE_UNINIT = MEM_UNINIT | ARG_PTR_TO_MAP_VALUE,
seeing how this "alias" is used only in few places, I'd just use
`ARG_PTR_TO_MAP_VALUE | MEM_UNINIT` directly in prototype declaration
and the MEM_UNINIT flag directly in verifier logic.
> + /* pointer to memory does not need to be initialized, helper function must fill
> + * all bytes or clear them in error case.
> + */
> + ARG_PTR_TO_MEM_UNINIT = MEM_UNINIT | ARG_PTR_TO_MEM,
>
> /* This must be the last entry. Its purpose is to ensure the enum is
> * wide enough to hold the higher bits reserved for bpf_type_flag.
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d175b70067b3..90280d5666be 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5136,8 +5136,7 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
>
> static bool arg_type_is_mem_ptr(enum bpf_arg_type type)
> {
> - return base_type(type) == ARG_PTR_TO_MEM ||
> - base_type(type) == ARG_PTR_TO_UNINIT_MEM;
> + return base_type(type) == ARG_PTR_TO_MEM;
> }
Is this helper function even useful anymore? I'd just drop this
function altogether.
>
> static bool arg_type_is_mem_size(enum bpf_arg_type type)
> @@ -5273,7 +5272,6 @@ static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE }
> static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> [ARG_PTR_TO_MAP_KEY] = &map_key_value_types,
> [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types,
> - [ARG_PTR_TO_UNINIT_MAP_VALUE] = &map_key_value_types,
> [ARG_CONST_SIZE] = &scalar_types,
> [ARG_CONST_SIZE_OR_ZERO] = &scalar_types,
> [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types,
> @@ -5287,7 +5285,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
> [ARG_PTR_TO_BTF_ID] = &btf_ptr_types,
> [ARG_PTR_TO_SPIN_LOCK] = &spin_lock_types,
> [ARG_PTR_TO_MEM] = &mem_types,
> - [ARG_PTR_TO_UNINIT_MEM] = &mem_types,
> [ARG_PTR_TO_ALLOC_MEM] = &alloc_mem_types,
> [ARG_PTR_TO_INT] = &int_ptr_types,
> [ARG_PTR_TO_LONG] = &int_ptr_types,
> @@ -5451,8 +5448,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> return -EACCES;
> }
>
> - if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> + if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
> err = resolve_map_arg_type(env, meta, &arg_type);
> if (err)
> return err;
> @@ -5528,8 +5524,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> err = check_helper_mem_access(env, regno,
> meta->map_ptr->key_size, false,
> NULL);
> - } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> - base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> + } else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
> if (type_may_be_null(arg_type) && register_is_null(reg))
> return 0;
>
> @@ -5541,7 +5536,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> verbose(env, "invalid map_ptr to access map->value\n");
> return -EACCES;
> }
> - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
> + meta->raw_mode = (arg_type == ARG_PTR_TO_MAP_VALUE_UNINIT);
> err = check_helper_mem_access(env, regno,
> meta->map_ptr->value_size, false,
> meta);
> @@ -5572,7 +5567,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> /* The access to this pointer is only checked when we hit the
> * next is_mem_size argument below.
> */
> - meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
> + meta->raw_mode = (arg_type == ARG_PTR_TO_MEM_UNINIT);
aside: raw_mode is a horrible name that communicates literally nothing
towards its semantics (IMO), would be nice to fix that, I'm always
confused by it
> } else if (arg_type_is_mem_size(arg_type)) {
> bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
>
[...]
> @@ -1406,7 +1406,7 @@ static const struct bpf_func_proto bpf_get_stack_proto_tp = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_UNINIT_MEM,
> + .arg2_type = ARG_PTR_TO_MEM_UNINIT,
> .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> .arg4_type = ARG_ANYTHING,
> };
> @@ -1473,7 +1473,7 @@ static const struct bpf_func_proto bpf_perf_prog_read_value_proto = {
> .gpl_only = true,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_PTR_TO_CTX,
> - .arg2_type = ARG_PTR_TO_UNINIT_MEM,
> + .arg2_type = ARG_PTR_TO_MEM_UNINIT,
indentation is off
> .arg3_type = ARG_CONST_SIZE,
> };
>
[...]
