diff mbox series

[RFC,bpf-next,v2,3/9] bpf: Replace RET_XXX_OR_NULL with RET_XXX | PTR_MAYBE_NULL

Message ID 20211130012948.380602-4-haoluo@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Introduce composable bpf types | expand

Checks

Context Check Description
bpf/vmtest-bpf-next fail VM_Test
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 12421 this patch: 12423
netdev/cc_maintainers warning 2 maintainers not CCed: netdev@vger.kernel.org john.fastabend@gmail.com
netdev/build_clang fail Errors and warnings before: 2094 this patch: 2097
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 11586 this patch: 11588
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Hao Luo Nov. 30, 2021, 1:29 a.m. UTC 
We have introduced a new type to make bpf_ret composable, by
introducing flags to tag a particular type.

One of the flag is PTR_MAYBE_NULL, which indicates a pointer
may be NULL. This patch switches the qualified arg_types to
use this flag. The ret_types changed in this patch include:

1. RET_PTR_TO_MAP_VALUE_OR_NULL
2. RET_PTR_TO_SOCKET_OR_NULL
3. RET_PTR_TO_TCP_SOCK_OR_NULL
4. RET_PTR_TO_SOCK_COMMON_OR_NULL
5. RET_PTR_TO_ALLOC_MEM_OR_NULL
6. RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL
7. RET_PTR_TO_BTF_ID_OR_NULL

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   | 19 ++++++++++------
 kernel/bpf/helpers.c  |  2 +-
 kernel/bpf/verifier.c | 53 +++++++++++++++++++++++--------------------
 3 files changed, 42 insertions(+), 32 deletions(-)

Comments

Alexei Starovoitov Dec. 1, 2021, 8:30 p.m. UTC | #1 
On Mon, Nov 29, 2021 at 05:29:42PM -0800, Hao Luo wrote:
>  	/* update return register (already marked as written above) */
> -	if (fn->ret_type == RET_INTEGER) {
> +	ret_type = fn->ret_type;
> +	if (ret_type == RET_INTEGER) {
>  		/* sets type to SCALAR_VALUE */
>  		mark_reg_unknown(env, regs, BPF_REG_0);
> -	} else if (fn->ret_type == RET_VOID) {
> +	} else if (ret_type == RET_VOID) {
>  		regs[BPF_REG_0].type = NOT_INIT;
> -	} else if (fn->ret_type == RET_PTR_TO_MAP_VALUE_OR_NULL ||
> -		   fn->ret_type == RET_PTR_TO_MAP_VALUE) {
> +	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_MAP_VALUE) {
>  		/* There is no offset yet applied, variable or fixed */
>  		mark_reg_known_zero(env, regs, BPF_REG_0);
>  		/* remember map_ptr, so that check_map_access()
> @@ -6530,28 +6536,27 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		}
>  		regs[BPF_REG_0].map_ptr = meta.map_ptr;
>  		regs[BPF_REG_0].map_uid = meta.map_uid;
> -		if (fn->ret_type == RET_PTR_TO_MAP_VALUE) {
> +		if (ret_type_may_be_null(fn->ret_type)) {

it should have been ret_type here?

> +			regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL;
> +		} else {
>  			regs[BPF_REG_0].type = PTR_TO_MAP_VALUE;
Hao Luo Dec. 1, 2021, 10:40 p.m. UTC | #2 
On Wed, Dec 1, 2021 at 12:30 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 29, 2021 at 05:29:42PM -0800, Hao Luo wrote:
> >       /* update return register (already marked as written above) */
> > -     if (fn->ret_type == RET_INTEGER) {
> > +     ret_type = fn->ret_type;
> > +     if (ret_type == RET_INTEGER) {
> >               /* sets type to SCALAR_VALUE */
> >               mark_reg_unknown(env, regs, BPF_REG_0);
> > -     } else if (fn->ret_type == RET_VOID) {
> > +     } else if (ret_type == RET_VOID) {
> >               regs[BPF_REG_0].type = NOT_INIT;
> > -     } else if (fn->ret_type == RET_PTR_TO_MAP_VALUE_OR_NULL ||
> > -                fn->ret_type == RET_PTR_TO_MAP_VALUE) {
> > +     } else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_MAP_VALUE) {
> >               /* There is no offset yet applied, variable or fixed */
> >               mark_reg_known_zero(env, regs, BPF_REG_0);
> >               /* remember map_ptr, so that check_map_access()
> > @@ -6530,28 +6536,27 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >               }
> >               regs[BPF_REG_0].map_ptr = meta.map_ptr;
> >               regs[BPF_REG_0].map_uid = meta.map_uid;
> > -             if (fn->ret_type == RET_PTR_TO_MAP_VALUE) {
> > +             if (ret_type_may_be_null(fn->ret_type)) {
>
> it should have been ret_type here?
>

Yes. I think fn->ret_type and ret_type are the same here. I can
replace it with 'ret_type' in the next version.

> > +                     regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL;
> > +             } else {
> >                       regs[BPF_REG_0].type = PTR_TO_MAP_VALUE;
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index da31c79724d4..d6189eb14a1f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -390,17 +390,22 @@  enum bpf_return_type {
 	RET_INTEGER,			/* function returns integer */
 	RET_VOID,			/* function doesn't return anything */
 	RET_PTR_TO_MAP_VALUE,		/* returns a pointer to map elem value */
-	RET_PTR_TO_MAP_VALUE_OR_NULL,	/* returns a pointer to map elem value or NULL */
-	RET_PTR_TO_SOCKET_OR_NULL,	/* returns a pointer to a socket or NULL */
-	RET_PTR_TO_TCP_SOCK_OR_NULL,	/* returns a pointer to a tcp_sock or NULL */
-	RET_PTR_TO_SOCK_COMMON_OR_NULL,	/* returns a pointer to a sock_common or NULL */
-	RET_PTR_TO_ALLOC_MEM_OR_NULL,	/* returns a pointer to dynamically allocated memory or NULL */
-	RET_PTR_TO_BTF_ID_OR_NULL,	/* returns a pointer to a btf_id or NULL */
-	RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, /* returns a pointer to a valid memory or a btf_id or NULL */
+	RET_PTR_TO_SOCKET,		/* returns a pointer to a socket */
+	RET_PTR_TO_TCP_SOCK,		/* returns a pointer to a tcp_sock */
+	RET_PTR_TO_SOCK_COMMON,		/* returns a pointer to a sock_common */
+	RET_PTR_TO_ALLOC_MEM,		/* returns a pointer to dynamically allocated memory */
 	RET_PTR_TO_MEM_OR_BTF_ID,	/* returns a pointer to a valid memory or a btf_id */
 	RET_PTR_TO_BTF_ID,		/* returns a pointer to a btf_id */
 	__BPF_RET_TYPE_MAX,
 
+	/* Extended ret_types. */
+	RET_PTR_TO_MAP_VALUE_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_MAP_VALUE,
+	RET_PTR_TO_SOCKET_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_SOCKET,
+	RET_PTR_TO_TCP_SOCK_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_TCP_SOCK,
+	RET_PTR_TO_SOCK_COMMON_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_SOCK_COMMON,
+	RET_PTR_TO_ALLOC_MEM_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_ALLOC_MEM,
+	RET_PTR_TO_BTF_ID_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_BTF_ID,
+
 	/* 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/helpers.c b/kernel/bpf/helpers.c
index 1ffd469c217f..293d9314ec7f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -667,7 +667,7 @@  BPF_CALL_2(bpf_per_cpu_ptr, const void *, ptr, u32, cpu)
 const struct bpf_func_proto bpf_per_cpu_ptr_proto = {
 	.func		= bpf_per_cpu_ptr,
 	.gpl_only	= false,
-	.ret_type	= RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL,
+	.ret_type	= RET_PTR_TO_MEM_OR_BTF_ID | PTR_MAYBE_NULL,
 	.arg1_type	= ARG_PTR_TO_PERCPU_BTF_ID,
 	.arg2_type	= ARG_ANYTHING,
 };
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 78227d2cffb1..fb44db2456f2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -477,6 +477,11 @@  static bool arg_type_may_be_null(enum bpf_arg_type type)
 	return BPF_TYPE_FLAG(type) & PTR_MAYBE_NULL;
 }
 
+static bool ret_type_may_be_null(enum bpf_return_type type)
+{
+	return BPF_TYPE_FLAG(type) & PTR_MAYBE_NULL;
+}
+
 /* Determine whether the function releases some resources allocated by another
  * function call. The first reference type argument will be assumed to be
  * released by release_reference().
@@ -6370,6 +6375,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 			     int *insn_idx_p)
 {
 	const struct bpf_func_proto *fn = NULL;
+	enum bpf_return_type ret_type;
 	struct bpf_reg_state *regs;
 	struct bpf_call_arg_meta meta;
 	int insn_idx = *insn_idx_p;
@@ -6510,13 +6516,13 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 	regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG;
 
 	/* update return register (already marked as written above) */
-	if (fn->ret_type == RET_INTEGER) {
+	ret_type = fn->ret_type;
+	if (ret_type == RET_INTEGER) {
 		/* sets type to SCALAR_VALUE */
 		mark_reg_unknown(env, regs, BPF_REG_0);
-	} else if (fn->ret_type == RET_VOID) {
+	} else if (ret_type == RET_VOID) {
 		regs[BPF_REG_0].type = NOT_INIT;
-	} else if (fn->ret_type == RET_PTR_TO_MAP_VALUE_OR_NULL ||
-		   fn->ret_type == RET_PTR_TO_MAP_VALUE) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_MAP_VALUE) {
 		/* There is no offset yet applied, variable or fixed */
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		/* remember map_ptr, so that check_map_access()
@@ -6530,28 +6536,27 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		}
 		regs[BPF_REG_0].map_ptr = meta.map_ptr;
 		regs[BPF_REG_0].map_uid = meta.map_uid;
-		if (fn->ret_type == RET_PTR_TO_MAP_VALUE) {
+		if (ret_type_may_be_null(fn->ret_type)) {
+			regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL;
+		} else {
 			regs[BPF_REG_0].type = PTR_TO_MAP_VALUE;
 			if (map_value_has_spin_lock(meta.map_ptr))
 				regs[BPF_REG_0].id = ++env->id_gen;
-		} else {
-			regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL;
 		}
-	} else if (fn->ret_type == RET_PTR_TO_SOCKET_OR_NULL) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_SOCKET) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_SOCKET_OR_NULL;
-	} else if (fn->ret_type == RET_PTR_TO_SOCK_COMMON_OR_NULL) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_SOCK_COMMON) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_SOCK_COMMON_OR_NULL;
-	} else if (fn->ret_type == RET_PTR_TO_TCP_SOCK_OR_NULL) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_TCP_SOCK) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_TCP_SOCK_OR_NULL;
-	} else if (fn->ret_type == RET_PTR_TO_ALLOC_MEM_OR_NULL) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_ALLOC_MEM) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
 		regs[BPF_REG_0].mem_size = meta.mem_size;
-	} else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL ||
-		   fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_MEM_OR_BTF_ID) {
 		const struct btf_type *t;
 
 		mark_reg_known_zero(env, regs, BPF_REG_0);
@@ -6570,28 +6575,28 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 				return -EINVAL;
 			}
 			regs[BPF_REG_0].type =
-				fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ?
-				PTR_TO_MEM : PTR_TO_MEM_OR_NULL;
+				(ret_type & PTR_MAYBE_NULL) ?
+				PTR_TO_MEM_OR_NULL : PTR_TO_MEM;
 			regs[BPF_REG_0].mem_size = tsize;
 		} else {
 			regs[BPF_REG_0].type =
-				fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID ?
-				PTR_TO_BTF_ID : PTR_TO_BTF_ID_OR_NULL;
+				(ret_type & PTR_MAYBE_NULL) ?
+				PTR_TO_BTF_ID_OR_NULL : PTR_TO_BTF_ID;
 			regs[BPF_REG_0].btf = meta.ret_btf;
 			regs[BPF_REG_0].btf_id = meta.ret_btf_id;
 		}
-	} else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL ||
-		   fn->ret_type == RET_PTR_TO_BTF_ID) {
+	} else if (BPF_BASE_TYPE(ret_type) == RET_PTR_TO_BTF_ID) {
 		int ret_btf_id;
 
 		mark_reg_known_zero(env, regs, BPF_REG_0);
-		regs[BPF_REG_0].type = fn->ret_type == RET_PTR_TO_BTF_ID ?
-						     PTR_TO_BTF_ID :
-						     PTR_TO_BTF_ID_OR_NULL;
+		regs[BPF_REG_0].type = (ret_type & PTR_MAYBE_NULL) ?
+						     PTR_TO_BTF_ID_OR_NULL :
+						     PTR_TO_BTF_ID;
 		ret_btf_id = *fn->ret_btf_id;
 		if (ret_btf_id == 0) {
 			verbose(env, "invalid return type %d of func %s#%d\n",
-				fn->ret_type, func_id_name(func_id), func_id);
+				BPF_BASE_TYPE(ret_type), func_id_name(func_id),
+				func_id);
 			return -EINVAL;
 		}
 		/* current BPF helper definitions are only coming from
@@ -6601,7 +6606,7 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		regs[BPF_REG_0].btf_id = ret_btf_id;
 	} else {
 		verbose(env, "unknown return type %d of func %s#%d\n",
-			fn->ret_type, func_id_name(func_id), func_id);
+			BPF_BASE_TYPE(ret_type), func_id_name(func_id), func_id);
 		return -EINVAL;
 	}