[RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Hao Luo]

This is a pure cleanup patchset that tries to use flag to mark whether
an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
so allows us to embed the properties of arguments in the struct, which
is a more scalable solution than introducing a new enum. This patchset
performs this transformation only on arg_type. If it looks good,
follow-up patches will do the same on reg_type and ret_type.

The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.

This is purely refactoring patch and should not have any behavior
changes.

Hao Luo (9):
  bpf: Replace enum bpf_arg_type with struct.
  bpf: Remove ARG_PTR_TO_MAP_VALUE_OR_NULL
  bpf: Remove ARG_PTR_TO_MEM_OR_NULL
  bpf: Remove ARG_CONST_SIZE_OR_ZERO
  bpf: Remove ARG_PTR_TO_CTX_OR_NULL
  bpf: Remove ARG_PTR_TO_SOCKET_OR_NULL
  bpf: Remove ARG_PTR_TO_ALLOC_MEM_OR_NULL
  bpf: Rename ARG_CONST_ALLOC_SIZE_OR_ZERO
  bpf: Rename ARG_PTR_TO_STACK_OR_NULL

 include/linux/bpf.h            | 102 ++---
 kernel/bpf/bpf_inode_storage.c |  15 +-
 kernel/bpf/bpf_iter.c          |  11 +-
 kernel/bpf/bpf_lsm.c           |  10 +-
 kernel/bpf/bpf_task_storage.c  |  15 +-
 kernel/bpf/btf.c               |   8 +-
 kernel/bpf/cgroup.c            |  31 +-
 kernel/bpf/core.c              |   8 +-
 kernel/bpf/helpers.c           | 138 ++++---
 kernel/bpf/ringbuf.c           |  32 +-
 kernel/bpf/stackmap.c          |  45 +-
 kernel/bpf/syscall.c           |  16 +-
 kernel/bpf/task_iter.c         |  13 +-
 kernel/bpf/verifier.c          | 179 ++++----
 kernel/trace/bpf_trace.c       | 267 +++++++-----
 net/core/bpf_sk_storage.c      |  41 +-
 net/core/filter.c              | 729 +++++++++++++++++----------------
 net/core/sock_map.c            |  48 +--
 net/ipv4/bpf_tcp_ca.c          |   4 +-
 19 files changed, 932 insertions(+), 780 deletions(-)

-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 2/9] bpf: Remove ARG_PTR_TO_MAP_VALUE_OR_NULL

[Hao Luo]

Remove ARG_PTR_TO_MAP_VALUE_OR_NULL and use flag to mark that
the argument may be null.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h            |  6 +++++-
 kernel/bpf/bpf_inode_storage.c |  5 ++++-
 kernel/bpf/bpf_task_storage.c  |  5 ++++-
 kernel/bpf/verifier.c          | 11 +++++------
 net/core/bpf_sk_storage.c      | 15 ++++++++++++---
 5 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 287d819e73f8..d8de8f00e40d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -297,6 +297,9 @@ bool bpf_map_meta_equal(const struct bpf_map *meta0,
 
 extern const struct bpf_map_ops bpf_map_offload_ops;
 
+/* argument may be null or zero */
+#define ARG_FLAG_MAYBE_NULL	1
+
 /* function argument constraints */
 struct bpf_arg_type {
 	enum {
@@ -309,7 +312,6 @@ struct bpf_arg_type {
 		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 */
-		ARG_PTR_TO_MAP_VALUE_OR_NULL,	/* pointer to stack used as map value or NULL */
 
 		/* the following constraints used to prototype bpf_memcmp() and other
 		 * functions that access data on eBPF program stack
@@ -345,6 +347,8 @@ struct bpf_arg_type {
 		ARG_PTR_TO_TIMER,	/* pointer to bpf_timer */
 		__BPF_ARG_TYPE_MAX,
 	} type;
+
+	u8 flag;
 };
 
 /* type of values returned from helper functions */
diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index 091352613225..acb566a3b37f 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -265,7 +265,10 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
 	.arg1		= { .type = ARG_CONST_MAP_PTR, }
 	.arg2		= { .type = ARG_PTR_TO_BTF_ID, }
 	.arg2_btf_id	= &bpf_inode_storage_btf_ids[0],
-	.arg3		= { .type = ARG_PTR_TO_MAP_VALUE_OR_NULL, }
+	.arg3		= {
+		.type = ARG_PTR_TO_MAP_VALUE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	}
 	.arg4_type	= { .type = ARG_ANYTHING, }
 };
 
diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
index 04bb681edc78..bdc0925c2fd3 100644
--- a/kernel/bpf/bpf_task_storage.c
+++ b/kernel/bpf/bpf_task_storage.c
@@ -324,7 +324,10 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
 	.arg1	 = { .type = ARG_CONST_MAP_PTR },
 	.arg2	 = { .type = ARG_PTR_TO_BTF_ID },
 	.arg2_btf_id = &btf_task_struct_ids[0],
-	.arg3	 = { .type = ARG_PTR_TO_MAP_VALUE_OR_NULL },
+	.arg3	 = {
+		.type = ARG_PTR_TO_MAP_VALUE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4	 = { .type = ARG_ANYTHING },
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1f2aaa2214d9..f55967f92d22 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -480,7 +480,7 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
-	return arg.type == ARG_PTR_TO_MAP_VALUE_OR_NULL ||
+	return arg.flag & ARG_FLAG_MAYBE_NULL ||
 	       arg.type == ARG_PTR_TO_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_CTX_OR_NULL ||
 	       arg.type == ARG_PTR_TO_SOCKET_OR_NULL ||
@@ -5089,7 +5089,6 @@ 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_PTR_TO_MAP_VALUE_OR_NULL]	= &map_key_value_types,
 	[ARG_CONST_SIZE]		= &scalar_types,
 	[ARG_CONST_SIZE_OR_ZERO]	= &scalar_types,
 	[ARG_CONST_ALLOC_SIZE_OR_ZERO]	= &scalar_types,
@@ -5209,8 +5208,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg_num,
 	}
 
 	if (arg.type == ARG_PTR_TO_MAP_VALUE ||
-	    arg.type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
-	    arg.type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
+	    arg.type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
 		err = resolve_map_arg_type(env, meta, &arg);
 		if (err)
 			return err;
@@ -5286,9 +5284,10 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg_num,
 					      meta->map_ptr->key_size, false,
 					      NULL);
 	} else if (arg.type == ARG_PTR_TO_MAP_VALUE ||
-		   (arg.type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
-		    !register_is_null(reg)) ||
 		   arg.type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
+		if ((arg.flag & ARG_FLAG_MAYBE_NULL) && register_is_null(reg))
+			return err;
+
 		/* bpf_map_xxx(..., map_ptr, ..., value) call:
 		 * check [value, value + map->value_size) validity
 		 */
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 81f8529c0169..22fdbe3d68e3 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -357,7 +357,10 @@ const struct bpf_func_proto bpf_sk_storage_get_proto = {
 	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
 	.arg2		= { .type = ARG_PTR_TO_BTF_ID_SOCK_COMMON },
-	.arg3		= { .type = ARG_PTR_TO_MAP_VALUE_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_MAP_VALUE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -367,7 +370,10 @@ const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto = {
 	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
 	.arg2		= { .type = ARG_PTR_TO_CTX }, /* context is 'struct sock' */
-	.arg3		= { .type = ARG_PTR_TO_MAP_VALUE_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_MAP_VALUE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -438,7 +444,10 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
 	.arg2		= { .type = ARG_PTR_TO_BTF_ID },
 	.arg2_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
-	.arg3		= { .type = ARG_PTR_TO_MAP_VALUE_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_MAP_VALUE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 	.allowed	= bpf_sk_storage_tracing_allowed,
 };
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 3/9] bpf: Remove ARG_PTR_TO_MEM_OR_NULL

[Hao Luo]

Remove ARG_PTR_TO_MEM_OR_NULL and use flag to mark that the
argument may be null.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h      |  1 -
 kernel/bpf/helpers.c     | 10 ++++++++--
 kernel/bpf/verifier.c    |  3 ---
 kernel/trace/bpf_trace.c | 15 ++++++++++++---
 net/core/filter.c        | 15 ++++++++++++---
 5 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d8de8f00e40d..dd92418814b5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -317,7 +317,6 @@ struct bpf_arg_type {
 		 * functions that access data on eBPF program stack
 		 */
 		ARG_PTR_TO_MEM,		/* pointer to valid memory (stack, packet, map value) */
-		ARG_PTR_TO_MEM_OR_NULL, /* pointer to valid memory or NULL */
 		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.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index d68c70f4b4b6..cd792777afb2 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1008,10 +1008,16 @@ const struct bpf_func_proto bpf_snprintf_proto = {
 	.func		= bpf_snprintf,
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
 	.arg3		= { .type = ARG_PTR_TO_CONST_STR },
-	.arg4		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg4		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f55967f92d22..eb69b8bddee5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -481,7 +481,6 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
 	return arg.flag & ARG_FLAG_MAYBE_NULL ||
-	       arg.type == ARG_PTR_TO_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_CTX_OR_NULL ||
 	       arg.type == ARG_PTR_TO_SOCKET_OR_NULL ||
 	       arg.type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
@@ -4951,7 +4950,6 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
 static bool arg_type_is_mem_ptr(struct bpf_arg_type arg)
 {
 	return arg.type == ARG_PTR_TO_MEM ||
-	       arg.type == ARG_PTR_TO_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_UNINIT_MEM;
 }
 
@@ -5104,7 +5102,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_MEM_OR_NULL]	= &mem_types,
 	[ARG_PTR_TO_UNINIT_MEM]		= &mem_types,
 	[ARG_PTR_TO_ALLOC_MEM]		= &alloc_mem_types,
 	[ARG_PTR_TO_ALLOC_MEM_OR_NULL]	= &alloc_mem_types,
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 6e0b17637787..086889a29ca7 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -452,7 +452,10 @@ static const struct bpf_func_proto bpf_trace_vprintk_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_MEM },
 	.arg2		= { .type = ARG_CONST_SIZE },
-	.arg3		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_CONST_SIZE_OR_ZERO },
 };
 
@@ -494,7 +497,10 @@ static const struct bpf_func_proto bpf_seq_printf_proto = {
 	.arg1_btf_id	= &btf_seq_file_ids[0],
 	.arg2		= { .type = ARG_PTR_TO_MEM },
 	.arg3		= { .type = ARG_CONST_SIZE },
-	.arg4		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg4		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
 };
 
@@ -1435,7 +1441,10 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
 	.gpl_only       = true,
 	.ret_type       = RET_INTEGER,
 	.arg1	      = { .type = ARG_PTR_TO_CTX },
-	.arg2	      = { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg2	      = {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3	      = { .type = ARG_CONST_SIZE_OR_ZERO },
 	.arg4	      = { .type = ARG_ANYTHING },
 };
diff --git a/net/core/filter.c b/net/core/filter.c
index 90fa7f67f3c2..81c6638ffc2a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2018,9 +2018,15 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
 	.gpl_only	= false,
 	.pkt_access	= true,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
-	.arg3		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_CONST_SIZE_OR_ZERO },
 	.arg5		= { .type = ARG_ANYTHING },
 };
@@ -2541,7 +2547,10 @@ static const struct bpf_func_proto bpf_redirect_neigh_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_ANYTHING },
-	.arg2		= { .type = ARG_PTR_TO_MEM_OR_NULL },
+	.arg2		= {
+		.type = ARG_PTR_TO_MEM,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
 	.arg4		= { .type = ARG_ANYTHING },
 };
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 4/9] bpf: Remove ARG_CONST_SIZE_OR_ZERO

[Hao Luo]

Remove ARG_CONST_SIZE_OR_ZERO and use flag to mark that the
argument may be zero.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h      |  2 -
 kernel/bpf/helpers.c     | 20 +++++++--
 kernel/bpf/ringbuf.c     |  5 ++-
 kernel/bpf/stackmap.c    | 15 +++++--
 kernel/bpf/verifier.c    |  8 ++--
 kernel/trace/bpf_trace.c | 90 ++++++++++++++++++++++++++++++++--------
 net/core/filter.c        | 35 ++++++++++++----
 7 files changed, 135 insertions(+), 40 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index dd92418814b5..27f81989f992 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -323,8 +323,6 @@ struct bpf_arg_type {
 					 */
 
 		ARG_CONST_SIZE,		/* number of bytes accessed from memory */
-		ARG_CONST_SIZE_OR_ZERO,	/* number of bytes accessed from memory or 0 */
-
 		ARG_PTR_TO_CTX,		/* pointer to context */
 		ARG_PTR_TO_CTX_OR_NULL,	/* pointer to context or NULL */
 		ARG_ANYTHING,		/* any (initialized) argument is ok */
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index cd792777afb2..082a8620f666 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -631,7 +631,10 @@ const struct bpf_func_proto bpf_event_output_data_proto =  {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_3(bpf_copy_from_user, void *, dst, u32, size,
@@ -652,7 +655,10 @@ const struct bpf_func_proto bpf_copy_from_user_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -1012,13 +1018,19 @@ const struct bpf_func_proto bpf_snprintf_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_PTR_TO_CONST_STR },
 	.arg4		= {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 /* BPF map elements can contain 'struct bpf_timer'.
diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index bf29de9c4a2d..a8af9c7c6423 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -445,7 +445,10 @@ const struct bpf_func_proto bpf_ringbuf_output_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
 	.arg2		= { .type = ARG_PTR_TO_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index ae7bfc278443..496baa1b8def 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -464,7 +464,10 @@ const struct bpf_func_proto bpf_get_stack_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_CTX },
 	.arg2		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -491,7 +494,10 @@ const struct bpf_func_proto bpf_get_task_stack_proto = {
 	.arg1		= { .type = ARG_PTR_TO_BTF_ID },
 	.arg1_btf_id	= &btf_task_struct_ids[0],
 	.arg2		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -554,7 +560,10 @@ const struct bpf_func_proto bpf_get_stack_proto_pe = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_CTX },
 	.arg2		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index eb69b8bddee5..1a4830ad2be2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2493,7 +2493,7 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx,
  * r5 += 1
  * ...
  * call bpf_perf_event_output#25
- *   where .arg5 = ARG_CONST_SIZE_OR_ZERO
+ *   where .arg5 = ARG_CONST_SIZE
  *
  * and this case:
  * r6 = 1
@@ -4955,8 +4955,7 @@ static bool arg_type_is_mem_ptr(struct bpf_arg_type arg)
 
 static bool arg_type_is_mem_size(struct bpf_arg_type arg)
 {
-	return arg.type == ARG_CONST_SIZE ||
-	       arg.type == ARG_CONST_SIZE_OR_ZERO;
+	return arg.type == ARG_CONST_SIZE;
 }
 
 static bool arg_type_is_alloc_size(struct bpf_arg_type arg)
@@ -5088,7 +5087,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[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,
 	[ARG_CONST_MAP_PTR]		= &const_map_ptr_types,
 	[ARG_PTR_TO_CTX]		= &context_types,
@@ -5326,7 +5324,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg_num,
 		 */
 		meta->raw_mode = (arg.type == ARG_PTR_TO_UNINIT_MEM);
 	} else if (arg_type_is_mem_size(arg)) {
-		bool zero_size_allowed = (arg.type == ARG_CONST_SIZE_OR_ZERO);
+		bool zero_size_allowed = (arg.flag & ARG_FLAG_MAYBE_NULL);
 
 		/* This is used to refine r0 return value bounds for helpers
 		 * that enforce this value as an upper bound on return values.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 086889a29ca7..7d6b51ed3292 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -171,7 +171,10 @@ const struct bpf_func_proto bpf_probe_read_user_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -208,7 +211,10 @@ const struct bpf_func_proto bpf_probe_read_user_str_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -234,7 +240,10 @@ const struct bpf_func_proto bpf_probe_read_kernel_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -269,7 +278,10 @@ const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -289,7 +301,10 @@ static const struct bpf_func_proto bpf_probe_read_compat_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
@@ -308,7 +323,10 @@ static const struct bpf_func_proto bpf_probe_read_compat_str_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 #endif /* CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE */
@@ -456,7 +474,10 @@ static const struct bpf_func_proto bpf_trace_vprintk_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg4		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg4		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 const struct bpf_func_proto *bpf_get_trace_vprintk_proto(void)
@@ -501,7 +522,10 @@ static const struct bpf_func_proto bpf_seq_printf_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_3(bpf_seq_write, struct seq_file *, m, const void *, data, u32, len)
@@ -516,7 +540,10 @@ static const struct bpf_func_proto bpf_seq_write_proto = {
 	.arg1		= { .type = ARG_PTR_TO_BTF_ID },
 	.arg1_btf_id	= &btf_seq_file_ids[0],
 	.arg2		= { .type = ARG_PTR_TO_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_4(bpf_seq_printf_btf, struct seq_file *, m, struct btf_ptr *, ptr,
@@ -540,7 +567,10 @@ static const struct bpf_func_proto bpf_seq_printf_btf_proto = {
 	.arg1		= { .type = ARG_PTR_TO_BTF_ID },
 	.arg1_btf_id	= &btf_seq_file_ids[0],
 	.arg2		= { .type = ARG_PTR_TO_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -701,7 +731,10 @@ static const struct bpf_func_proto bpf_perf_event_output_proto = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 static DEFINE_PER_CPU(int, bpf_event_output_nest_level);
@@ -952,7 +985,10 @@ static const struct bpf_func_proto bpf_d_path_proto = {
 	.arg1		= { .type = ARG_PTR_TO_BTF_ID },
 	.arg1_btf_id	= &bpf_d_path_btf_ids[0],
 	.arg2		= { .type = ARG_PTR_TO_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.allowed	= bpf_d_path_allowed,
 };
 
@@ -1094,7 +1130,10 @@ static const struct bpf_func_proto bpf_get_branch_snapshot_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 static const struct bpf_func_proto *
@@ -1296,7 +1335,10 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_tp = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_3(bpf_get_stackid_tp, void *, tp_buff, struct bpf_map *, map,
@@ -1337,7 +1379,10 @@ static const struct bpf_func_proto bpf_get_stack_proto_tp = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_CTX },
 	.arg2		= { .type = ARG_PTR_TO_UNINIT_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -1445,7 +1490,10 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg3	      = { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3	      = {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4	      = { .type = ARG_ANYTHING },
 };
 
@@ -1525,7 +1573,10 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 extern const struct bpf_func_proto bpf_skb_output_proto;
@@ -1579,7 +1630,10 @@ static const struct bpf_func_proto bpf_get_stack_proto_raw_tp = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_CTX },
 	.arg2		= { .type = ARG_PTR_TO_MEM },
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 81c6638ffc2a..5e0f726a9bcd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2022,12 +2022,18 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg2		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg4		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg4		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg5		= { .type = ARG_ANYTHING },
 };
 
@@ -2551,7 +2557,10 @@ static const struct bpf_func_proto bpf_redirect_neigh_proto = {
 		.type = ARG_PTR_TO_MEM,
 		.flag = ARG_FLAG_MAYBE_NULL,
 	},
-	.arg3		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg3		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
 
@@ -4184,7 +4193,10 @@ static const struct bpf_func_proto bpf_skb_event_output_proto = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BTF_ID_LIST_SINGLE(bpf_skb_output_btf_ids, struct, sk_buff)
@@ -4198,7 +4210,10 @@ const struct bpf_func_proto bpf_skb_output_proto = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 static unsigned short bpf_tunnel_key_af(u64 flags)
@@ -4577,7 +4592,10 @@ static const struct bpf_func_proto bpf_xdp_event_output_proto = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BTF_ID_LIST_SINGLE(bpf_xdp_output_btf_ids, struct, xdp_buff)
@@ -4591,7 +4609,10 @@ const struct bpf_func_proto bpf_xdp_output_proto = {
 	.arg2		= { .type = ARG_CONST_MAP_PTR },
 	.arg3		= { .type = ARG_ANYTHING },
 	.arg4		= { .type = ARG_PTR_TO_MEM },
-	.arg5		= { .type = ARG_CONST_SIZE_OR_ZERO },
+	.arg5		= {
+		.type = ARG_CONST_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [RFC PATCH bpf-next 4/9] bpf: Remove ARG_CONST_SIZE_OR_ZERO

[Andrii Nakryiko]

On Mon, Nov 8, 2021 at 6:16 PM Hao Luo <haoluo@google.com> wrote:
>
> Remove ARG_CONST_SIZE_OR_ZERO and use flag to mark that the
> argument may be zero.
>
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---
>  include/linux/bpf.h      |  2 -
>  kernel/bpf/helpers.c     | 20 +++++++--
>  kernel/bpf/ringbuf.c     |  5 ++-
>  kernel/bpf/stackmap.c    | 15 +++++--
>  kernel/bpf/verifier.c    |  8 ++--
>  kernel/trace/bpf_trace.c | 90 ++++++++++++++++++++++++++++++++--------
>  net/core/filter.c        | 35 ++++++++++++----
>  7 files changed, 135 insertions(+), 40 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index dd92418814b5..27f81989f992 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -323,8 +323,6 @@ struct bpf_arg_type {
>                                          */
>
>                 ARG_CONST_SIZE,         /* number of bytes accessed from memory */
> -               ARG_CONST_SIZE_OR_ZERO, /* number of bytes accessed from memory or 0 */
> -
>                 ARG_PTR_TO_CTX,         /* pointer to context */
>                 ARG_PTR_TO_CTX_OR_NULL, /* pointer to context or NULL */
>                 ARG_ANYTHING,           /* any (initialized) argument is ok */
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index cd792777afb2..082a8620f666 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -631,7 +631,10 @@ const struct bpf_func_proto bpf_event_output_data_proto =  {
>         .arg2           = { .type = ARG_CONST_MAP_PTR },
>         .arg3           = { .type = ARG_ANYTHING },
>         .arg4           = { .type = ARG_PTR_TO_MEM },
> -       .arg5           = { .type = ARG_CONST_SIZE_OR_ZERO },
> +       .arg5           = {
> +               .type = ARG_CONST_SIZE,
> +               .flag = ARG_FLAG_MAYBE_NULL,

wait, OR_NULL and OR_ZERO isn't exactly the same thing... why are we
conflating them?

> +       },
>  };
>
>  BPF_CALL_3(bpf_copy_from_user, void *, dst, u32, size,

[...]

[RFC PATCH bpf-next 5/9] bpf: Remove ARG_PTR_TO_CTX_OR_NULL

[Hao Luo]

Remove ARG_PTR_TO_CTX_OR_NULL and use flag to mark that the argument
may be null.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   |  1 -
 kernel/bpf/cgroup.c   |  5 ++++-
 kernel/bpf/verifier.c |  2 --
 net/core/filter.c     | 20 ++++++++++++++++----
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 27f81989f992..27bf974ea6b5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -324,7 +324,6 @@ struct bpf_arg_type {
 
 		ARG_CONST_SIZE,		/* number of bytes accessed from memory */
 		ARG_PTR_TO_CTX,		/* pointer to context */
-		ARG_PTR_TO_CTX_OR_NULL,	/* pointer to context or NULL */
 		ARG_ANYTHING,		/* any (initialized) argument is ok */
 		ARG_PTR_TO_SPIN_LOCK,	/* pointer to bpf_spin_lock */
 		ARG_PTR_TO_SOCK_COMMON,	/* pointer to sock_common */
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index ce4de222605f..a0b431be46f8 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1888,7 +1888,10 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sockopt_proto = {
 	.func		= bpf_get_netns_cookie_sockopt,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_CTX_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_CTX,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 #endif
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1a4830ad2be2..07b93bfd3518 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -481,7 +481,6 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
 	return arg.flag & ARG_FLAG_MAYBE_NULL ||
-	       arg.type == ARG_PTR_TO_CTX_OR_NULL ||
 	       arg.type == ARG_PTR_TO_SOCKET_OR_NULL ||
 	       arg.type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_STACK_OR_NULL;
@@ -5090,7 +5089,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_CONST_ALLOC_SIZE_OR_ZERO]	= &scalar_types,
 	[ARG_CONST_MAP_PTR]		= &const_map_ptr_types,
 	[ARG_PTR_TO_CTX]		= &context_types,
-	[ARG_PTR_TO_CTX_OR_NULL]	= &context_types,
 	[ARG_PTR_TO_SOCK_COMMON]	= &sock_types,
 #ifdef CONFIG_NET
 	[ARG_PTR_TO_BTF_ID_SOCK_COMMON]	= &btf_id_sock_common_types,
diff --git a/net/core/filter.c b/net/core/filter.c
index 5e0f726a9bcd..2575460c054f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4691,7 +4691,10 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sock_proto = {
 	.func		= bpf_get_netns_cookie_sock,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_CTX_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_CTX,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_1(bpf_get_netns_cookie_sock_addr, struct bpf_sock_addr_kern *, ctx)
@@ -4703,7 +4706,10 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sock_addr_proto = {
 	.func		= bpf_get_netns_cookie_sock_addr,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_CTX_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_CTX,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_1(bpf_get_netns_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx)
@@ -4715,7 +4721,10 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sock_ops_proto = {
 	.func		= bpf_get_netns_cookie_sock_ops,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_CTX_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_CTX,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_1(bpf_get_netns_cookie_sk_msg, struct sk_msg *, ctx)
@@ -4727,7 +4736,10 @@ static const struct bpf_func_proto bpf_get_netns_cookie_sk_msg_proto = {
 	.func		= bpf_get_netns_cookie_sk_msg,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
-	.arg1		= { .type = ARG_PTR_TO_CTX_OR_NULL },
+	.arg1		= {
+		.type = ARG_PTR_TO_CTX,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 };
 
 BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 6/9] bpf: Remove ARG_PTR_TO_SOCKET_OR_NULL

[Hao Luo]

Remove ARG_PTR_TO_SOCKET_OR_NULL and use flag to mark that the
argument may be null.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   | 1 -
 kernel/bpf/verifier.c | 2 --
 net/core/filter.c     | 5 ++++-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 27bf974ea6b5..94eb776c925a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -330,7 +330,6 @@ struct bpf_arg_type {
 		ARG_PTR_TO_INT,		/* pointer to int */
 		ARG_PTR_TO_LONG,	/* pointer to long */
 		ARG_PTR_TO_SOCKET,	/* pointer to bpf_sock (fullsock) */
-		ARG_PTR_TO_SOCKET_OR_NULL,	/* pointer to bpf_sock (fullsock) or NULL */
 		ARG_PTR_TO_BTF_ID,	/* pointer to in-kernel struct */
 		ARG_PTR_TO_ALLOC_MEM,	/* pointer to dynamically allocated memory */
 		ARG_PTR_TO_ALLOC_MEM_OR_NULL,	/* pointer to dynamically allocated memory or NULL */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 07b93bfd3518..f89cf2a59c82 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -481,7 +481,6 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
 	return arg.flag & ARG_FLAG_MAYBE_NULL ||
-	       arg.type == ARG_PTR_TO_SOCKET_OR_NULL ||
 	       arg.type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_STACK_OR_NULL;
 }
@@ -5094,7 +5093,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_BTF_ID_SOCK_COMMON]	= &btf_id_sock_common_types,
 #endif
 	[ARG_PTR_TO_SOCKET]		= &fullsock_types,
-	[ARG_PTR_TO_SOCKET_OR_NULL]	= &fullsock_types,
 	[ARG_PTR_TO_BTF_ID]		= &btf_ptr_types,
 	[ARG_PTR_TO_SPIN_LOCK]		= &spin_lock_types,
 	[ARG_PTR_TO_MEM]		= &mem_types,
diff --git a/net/core/filter.c b/net/core/filter.c
index 2575460c054f..3ad3595a191e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10489,7 +10489,10 @@ static const struct bpf_func_proto bpf_sk_lookup_assign_proto = {
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_PTR_TO_CTX },
-	.arg2		= { .type = ARG_PTR_TO_SOCKET_OR_NULL },
+	.arg2		= {
+		.type = ARG_PTR_TO_SOCKET,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 7/9] bpf: Remove ARG_PTR_TO_ALLOC_MEM_OR_NULL

[Hao Luo]

Remove ARG_PTR_TO_ALLOC_MEM_OR_NULL and use flag to mark
that the argument may be null. This ARG type doesn't seem to be
used anywhere.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   | 1 -
 kernel/bpf/verifier.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 94eb776c925a..3313ba544758 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -332,7 +332,6 @@ struct bpf_arg_type {
 		ARG_PTR_TO_SOCKET,	/* pointer to bpf_sock (fullsock) */
 		ARG_PTR_TO_BTF_ID,	/* pointer to in-kernel struct */
 		ARG_PTR_TO_ALLOC_MEM,	/* pointer to dynamically allocated memory */
-		ARG_PTR_TO_ALLOC_MEM_OR_NULL,	/* pointer to dynamically allocated memory or NULL */
 		ARG_CONST_ALLOC_SIZE_OR_ZERO,	/* number of allocated bytes requested */
 		ARG_PTR_TO_BTF_ID_SOCK_COMMON,	/* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */
 		ARG_PTR_TO_PERCPU_BTF_ID,	/* pointer to in-kernel percpu type */
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f89cf2a59c82..2e53605a051a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -481,7 +481,6 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
 	return arg.flag & ARG_FLAG_MAYBE_NULL ||
-	       arg.type == ARG_PTR_TO_ALLOC_MEM_OR_NULL ||
 	       arg.type == ARG_PTR_TO_STACK_OR_NULL;
 }
 
@@ -5098,7 +5097,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_MEM]		= &mem_types,
 	[ARG_PTR_TO_UNINIT_MEM]		= &mem_types,
 	[ARG_PTR_TO_ALLOC_MEM]		= &alloc_mem_types,
-	[ARG_PTR_TO_ALLOC_MEM_OR_NULL]	= &alloc_mem_types,
 	[ARG_PTR_TO_INT]		= &int_ptr_types,
 	[ARG_PTR_TO_LONG]		= &int_ptr_types,
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 8/9] bpf: Rename ARG_CONST_ALLOC_SIZE_OR_ZERO

[Hao Luo]

Rename ARG_CONST_ALLOC_SIZE_OR_ZERO to ARG_CONST_ALLOC_SIZE and
use flag to mark that the argument may be zero.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h   | 2 +-
 kernel/bpf/ringbuf.c  | 5 ++++-
 kernel/bpf/verifier.c | 4 ++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3313ba544758..a6dd5e85113d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -332,7 +332,7 @@ struct bpf_arg_type {
 		ARG_PTR_TO_SOCKET,	/* pointer to bpf_sock (fullsock) */
 		ARG_PTR_TO_BTF_ID,	/* pointer to in-kernel struct */
 		ARG_PTR_TO_ALLOC_MEM,	/* pointer to dynamically allocated memory */
-		ARG_CONST_ALLOC_SIZE_OR_ZERO,	/* number of allocated bytes requested */
+		ARG_CONST_ALLOC_SIZE,	/* number of allocated bytes requested */
 		ARG_PTR_TO_BTF_ID_SOCK_COMMON,	/* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */
 		ARG_PTR_TO_PERCPU_BTF_ID,	/* pointer to in-kernel percpu type */
 		ARG_PTR_TO_FUNC,	/* pointer to a bpf program function */
diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index a8af9c7c6423..132e03617881 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -363,7 +363,10 @@ const struct bpf_func_proto bpf_ringbuf_reserve_proto = {
 	.func		= bpf_ringbuf_reserve,
 	.ret_type	= RET_PTR_TO_ALLOC_MEM_OR_NULL,
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
-	.arg2		= { .type = ARG_CONST_ALLOC_SIZE_OR_ZERO },
+	.arg2		= {
+		.type = ARG_CONST_ALLOC_SIZE,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg3		= { .type = ARG_ANYTHING },
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2e53605a051a..9c4a8df25ef2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4957,7 +4957,7 @@ static bool arg_type_is_mem_size(struct bpf_arg_type arg)
 
 static bool arg_type_is_alloc_size(struct bpf_arg_type arg)
 {
-	return arg.type == ARG_CONST_ALLOC_SIZE_OR_ZERO;
+	return arg.type == ARG_CONST_ALLOC_SIZE;
 }
 
 static bool arg_type_is_int_ptr(struct bpf_arg_type arg)
@@ -5084,7 +5084,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[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_ALLOC_SIZE_OR_ZERO]	= &scalar_types,
+	[ARG_CONST_ALLOC_SIZE]		= &scalar_types,
 	[ARG_CONST_MAP_PTR]		= &const_map_ptr_types,
 	[ARG_PTR_TO_CTX]		= &context_types,
 	[ARG_PTR_TO_SOCK_COMMON]	= &sock_types,
-- 
2.34.0.rc0.344.g81b53c2807-goog

[RFC PATCH bpf-next 9/9] bpf: Rename ARG_PTR_TO_STACK_OR_NULL

[Hao Luo]

Rename ARG_PTR_TO_STACK_OR_NULL to ARG_PTR_TO_STACK and use flag
to mark that the argument may be null.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h    | 2 +-
 kernel/bpf/bpf_iter.c  | 5 ++++-
 kernel/bpf/task_iter.c | 5 ++++-
 kernel/bpf/verifier.c  | 5 ++---
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a6dd5e85113d..da4fa15127d6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -336,7 +336,7 @@ struct bpf_arg_type {
 		ARG_PTR_TO_BTF_ID_SOCK_COMMON,	/* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */
 		ARG_PTR_TO_PERCPU_BTF_ID,	/* pointer to in-kernel percpu type */
 		ARG_PTR_TO_FUNC,	/* pointer to a bpf program function */
-		ARG_PTR_TO_STACK_OR_NULL,	/* pointer to stack or NULL */
+		ARG_PTR_TO_STACK,	/* pointer to stack */
 		ARG_PTR_TO_CONST_STR,	/* pointer to a null terminated read-only string */
 		ARG_PTR_TO_TIMER,	/* pointer to bpf_timer */
 		__BPF_ARG_TYPE_MAX,
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 4fdf225c01f9..d3bded3e05d3 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -711,6 +711,9 @@ const struct bpf_func_proto bpf_for_each_map_elem_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1		= { .type = ARG_CONST_MAP_PTR },
 	.arg2		= { .type = ARG_PTR_TO_FUNC },
-	.arg3		= { .type = ARG_PTR_TO_STACK_OR_NULL },
+	.arg3		= {
+		.type = ARG_PTR_TO_STACK,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg4		= { .type = ARG_ANYTHING },
 };
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 0a5e3cf593b5..fc72701ae6b6 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -625,7 +625,10 @@ const struct bpf_func_proto bpf_find_vma_proto = {
 	.arg1_btf_id	= &btf_task_struct_ids[0],
 	.arg2		= { .type = ARG_ANYTHING },
 	.arg3		= { .type = ARG_PTR_TO_FUNC },
-	.arg4		= { .type = ARG_PTR_TO_STACK_OR_NULL },
+	.arg4		= {
+		.type = ARG_PTR_TO_STACK,
+		.flag = ARG_FLAG_MAYBE_NULL,
+	},
 	.arg5		= { .type = ARG_ANYTHING },
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9c4a8df25ef2..9c68b664f4f4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -480,8 +480,7 @@ static bool arg_type_may_be_refcounted(struct bpf_arg_type arg)
 
 static bool arg_type_may_be_null(struct bpf_arg_type arg)
 {
-	return arg.flag & ARG_FLAG_MAYBE_NULL ||
-	       arg.type == ARG_PTR_TO_STACK_OR_NULL;
+	return arg.flag & ARG_FLAG_MAYBE_NULL;
 }
 
 /* Determine whether the function releases some resources allocated by another
@@ -5101,7 +5100,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_LONG]		= &int_ptr_types,
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
 	[ARG_PTR_TO_FUNC]		= &func_ptr_types,
-	[ARG_PTR_TO_STACK_OR_NULL]	= &stack_ptr_types,
+	[ARG_PTR_TO_STACK]		= &stack_ptr_types,
 	[ARG_PTR_TO_CONST_STR]		= &const_str_ptr_types,
 	[ARG_PTR_TO_TIMER]		= &timer_types,
 };
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Alexei Starovoitov]

On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> This is a pure cleanup patchset that tries to use flag to mark whether
> an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> so allows us to embed the properties of arguments in the struct, which
> is a more scalable solution than introducing a new enum. This patchset
> performs this transformation only on arg_type. If it looks good,
> follow-up patches will do the same on reg_type and ret_type.
> 
> The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.

Nice. Thank you for working on it!

The enum->struct conversion works for bpf_arg_type, but applying
the same technique to bpf_reg_type could be problematic.
Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
Growing enum from 4 bytes to 8 byte struct will consume quite
a lot of extra memory.

>  19 files changed, 932 insertions(+), 780 deletions(-)

Just bpf_arg_type refactoring adds a lot of churn which could make
backports of future fixes not automatic anymore.
Similar converstion for bpf_reg_type and bpf_return_type will
be even more churn.
Have you considered using upper bits to represent flags?

Instead of diff:
-       .arg1_type      = ARG_CONST_MAP_PTR,
-       .arg2_type      = ARG_PTR_TO_FUNC,
-       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
-       .arg4_type      = ARG_ANYTHING,
+       .arg1           = { .type = ARG_CONST_MAP_PTR },
+       .arg2           = { .type = ARG_PTR_TO_FUNC },
+       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
+       .arg4           = { .type = ARG_ANYTHING },

can we make it look like:
       .arg1_type      = ARG_CONST_MAP_PTR,
       .arg2_type      = ARG_PTR_TO_FUNC,
-      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
+      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
       .arg4_type      = ARG_ANYTHING,

Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
would share the same flag bit: MAYBE_NULL.
Then static bool arg_type_may_be_null() will be comparing only single bit ?

While
        if (arg_type == ARG_PTR_TO_MAP_VALUE ||
            arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
            arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
will become:
        arg_type &= FLAG_MASK;
        if (arg_type == ARG_PTR_TO_MAP_VALUE ||
            arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {

Most of the time I would prefer explicit .type and .flag structure,
but saving memory is important for bpf_reg_type, so explicit bit
operations are probably justified.

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Hao Luo]

On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > This is a pure cleanup patchset that tries to use flag to mark whether
> > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > so allows us to embed the properties of arguments in the struct, which
> > is a more scalable solution than introducing a new enum. This patchset
> > performs this transformation only on arg_type. If it looks good,
> > follow-up patches will do the same on reg_type and ret_type.
> >
> > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
>
> Nice. Thank you for working on it!

No problem. :)

>
> The enum->struct conversion works for bpf_arg_type, but applying
> the same technique to bpf_reg_type could be problematic.
> Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> Growing enum from 4 bytes to 8 byte struct will consume quite
> a lot of extra memory.
>
> >  19 files changed, 932 insertions(+), 780 deletions(-)
>
> Just bpf_arg_type refactoring adds a lot of churn which could make
> backports of future fixes not automatic anymore.
> Similar converstion for bpf_reg_type and bpf_return_type will
> be even more churn.

Acknowledged.

> Have you considered using upper bits to represent flags?

Yes, I thought about that. Some of my thoughts are:

- I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
- What if we run out of flag bits in future?
- We could fold btf_id in the structure in this patchset. And new
fields could be easily added if needed.

So with these questions, I didn't pursue that approach in the first
place. But I admit that it does look better by writing

+      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,

Instead of

+       .arg3    = {
+               .type = ARG_PTR_TO_MAP_VALUE,
+               .flag = ARG_FLAG_MAYBE_NULL,
+       },

Let's see if there is any further comment. I can go take a look and
prepare for that approach in the next revision.



>
> Instead of diff:
> -       .arg1_type      = ARG_CONST_MAP_PTR,
> -       .arg2_type      = ARG_PTR_TO_FUNC,
> -       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> -       .arg4_type      = ARG_ANYTHING,
> +       .arg1           = { .type = ARG_CONST_MAP_PTR },
> +       .arg2           = { .type = ARG_PTR_TO_FUNC },
> +       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
> +       .arg4           = { .type = ARG_ANYTHING },
>
> can we make it look like:
>        .arg1_type      = ARG_CONST_MAP_PTR,
>        .arg2_type      = ARG_PTR_TO_FUNC,
> -      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
>        .arg4_type      = ARG_ANYTHING,
>
> Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> would share the same flag bit: MAYBE_NULL.
> Then static bool arg_type_may_be_null() will be comparing only single bit ?
>
> While
>         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
>             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
>             arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
> will become:
>         arg_type &= FLAG_MASK;
>         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
>             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
>
> Most of the time I would prefer explicit .type and .flag structure,
> but saving memory is important for bpf_reg_type, so explicit bit
> operations are probably justified.

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Andrii Nakryiko]

On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote:
>
> On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > > This is a pure cleanup patchset that tries to use flag to mark whether
> > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > > so allows us to embed the properties of arguments in the struct, which
> > > is a more scalable solution than introducing a new enum. This patchset
> > > performs this transformation only on arg_type. If it looks good,
> > > follow-up patches will do the same on reg_type and ret_type.
> > >
> > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
> >
> > Nice. Thank you for working on it!
>
> No problem. :)
>
> >
> > The enum->struct conversion works for bpf_arg_type, but applying
> > the same technique to bpf_reg_type could be problematic.
> > Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> > Growing enum from 4 bytes to 8 byte struct will consume quite
> > a lot of extra memory.
> >
> > >  19 files changed, 932 insertions(+), 780 deletions(-)
> >
> > Just bpf_arg_type refactoring adds a lot of churn which could make
> > backports of future fixes not automatic anymore.
> > Similar converstion for bpf_reg_type and bpf_return_type will
> > be even more churn.
>
> Acknowledged.
>
> > Have you considered using upper bits to represent flags?
>
> Yes, I thought about that. Some of my thoughts are:
>
> - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> - What if we run out of flag bits in future?
> - We could fold btf_id in the structure in this patchset. And new
> fields could be easily added if needed.
>
> So with these questions, I didn't pursue that approach in the first
> place. But I admit that it does look better by writing
>
> +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
>
> Instead of
>
> +       .arg3    = {
> +               .type = ARG_PTR_TO_MAP_VALUE,
> +               .flag = ARG_FLAG_MAYBE_NULL,
> +       },
>
> Let's see if there is any further comment. I can go take a look and
> prepare for that approach in the next revision.
>

+1 on staying within a single enum and using upper bits

>
>
> >
> > Instead of diff:
> > -       .arg1_type      = ARG_CONST_MAP_PTR,
> > -       .arg2_type      = ARG_PTR_TO_FUNC,
> > -       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > -       .arg4_type      = ARG_ANYTHING,
> > +       .arg1           = { .type = ARG_CONST_MAP_PTR },
> > +       .arg2           = { .type = ARG_PTR_TO_FUNC },
> > +       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
> > +       .arg4           = { .type = ARG_ANYTHING },
> >
> > can we make it look like:
> >        .arg1_type      = ARG_CONST_MAP_PTR,
> >        .arg2_type      = ARG_PTR_TO_FUNC,
> > -      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> >        .arg4_type      = ARG_ANYTHING,
> >
> > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> > would share the same flag bit: MAYBE_NULL.

I support using the same bit value, but should we use the exact same
enum name for three different enums? Like MAYBE_NULL, which enum is it
defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
REG_MAYBE_NULL be more explicit about what they apply to?

BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
the same thing, are they? Is the plan to use two different bits for
them or pretend that CONST_OR_ZERO "may be null"?

> > Then static bool arg_type_may_be_null() will be comparing only single bit ?
> >
> > While
> >         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> >             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> >             arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
> > will become:
> >         arg_type &= FLAG_MASK;
> >         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> >             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> >
> > Most of the time I would prefer explicit .type and .flag structure,
> > but saving memory is important for bpf_reg_type, so explicit bit
> > operations are probably justified.

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Andrii Nakryiko]

On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > > > This is a pure cleanup patchset that tries to use flag to mark whether
> > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > > > so allows us to embed the properties of arguments in the struct, which
> > > > is a more scalable solution than introducing a new enum. This patchset
> > > > performs this transformation only on arg_type. If it looks good,
> > > > follow-up patches will do the same on reg_type and ret_type.
> > > >
> > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
> > >
> > > Nice. Thank you for working on it!
> >
> > No problem. :)
> >
> > >
> > > The enum->struct conversion works for bpf_arg_type, but applying
> > > the same technique to bpf_reg_type could be problematic.
> > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> > > Growing enum from 4 bytes to 8 byte struct will consume quite
> > > a lot of extra memory.
> > >
> > > >  19 files changed, 932 insertions(+), 780 deletions(-)
> > >
> > > Just bpf_arg_type refactoring adds a lot of churn which could make
> > > backports of future fixes not automatic anymore.
> > > Similar converstion for bpf_reg_type and bpf_return_type will
> > > be even more churn.
> >
> > Acknowledged.
> >
> > > Have you considered using upper bits to represent flags?
> >
> > Yes, I thought about that. Some of my thoughts are:
> >
> > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> > - What if we run out of flag bits in future?
> > - We could fold btf_id in the structure in this patchset. And new
> > fields could be easily added if needed.
> >
> > So with these questions, I didn't pursue that approach in the first
> > place. But I admit that it does look better by writing
> >
> > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> >
> > Instead of
> >
> > +       .arg3    = {
> > +               .type = ARG_PTR_TO_MAP_VALUE,
> > +               .flag = ARG_FLAG_MAYBE_NULL,
> > +       },
> >
> > Let's see if there is any further comment. I can go take a look and
> > prepare for that approach in the next revision.
> >
>
> +1 on staying within a single enum and using upper bits
>
> >
> >
> > >
> > > Instead of diff:
> > > -       .arg1_type      = ARG_CONST_MAP_PTR,
> > > -       .arg2_type      = ARG_PTR_TO_FUNC,
> > > -       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > -       .arg4_type      = ARG_ANYTHING,
> > > +       .arg1           = { .type = ARG_CONST_MAP_PTR },
> > > +       .arg2           = { .type = ARG_PTR_TO_FUNC },
> > > +       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
> > > +       .arg4           = { .type = ARG_ANYTHING },
> > >
> > > can we make it look like:
> > >        .arg1_type      = ARG_CONST_MAP_PTR,
> > >        .arg2_type      = ARG_PTR_TO_FUNC,
> > > -      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> > >        .arg4_type      = ARG_ANYTHING,
> > >
> > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> > > would share the same flag bit: MAYBE_NULL.
>
> I support using the same bit value, but should we use the exact same
> enum name for three different enums? Like MAYBE_NULL, which enum is it
> defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
> REG_MAYBE_NULL be more explicit about what they apply to?

argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition
to REG_MAYBE_NULL".

>
> BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
> the same thing, are they? Is the plan to use two different bits for
> them or pretend that CONST_OR_ZERO "may be null"?
>
> > > Then static bool arg_type_may_be_null() will be comparing only single bit ?
> > >
> > > While
> > >         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> > >             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
> > >             arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
> > > will become:
> > >         arg_type &= FLAG_MASK;
> > >         if (arg_type == ARG_PTR_TO_MAP_VALUE ||
> > >             arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> > >
> > > Most of the time I would prefer explicit .type and .flag structure,
> > > but saving memory is important for bpf_reg_type, so explicit bit
> > > operations are probably justified.

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Alexei Starovoitov]

On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > > > > This is a pure cleanup patchset that tries to use flag to mark whether
> > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > > > > so allows us to embed the properties of arguments in the struct, which
> > > > > is a more scalable solution than introducing a new enum. This patchset
> > > > > performs this transformation only on arg_type. If it looks good,
> > > > > follow-up patches will do the same on reg_type and ret_type.
> > > > >
> > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
> > > >
> > > > Nice. Thank you for working on it!
> > >
> > > No problem. :)
> > >
> > > >
> > > > The enum->struct conversion works for bpf_arg_type, but applying
> > > > the same technique to bpf_reg_type could be problematic.
> > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> > > > Growing enum from 4 bytes to 8 byte struct will consume quite
> > > > a lot of extra memory.
> > > >
> > > > >  19 files changed, 932 insertions(+), 780 deletions(-)
> > > >
> > > > Just bpf_arg_type refactoring adds a lot of churn which could make
> > > > backports of future fixes not automatic anymore.
> > > > Similar converstion for bpf_reg_type and bpf_return_type will
> > > > be even more churn.
> > >
> > > Acknowledged.
> > >
> > > > Have you considered using upper bits to represent flags?
> > >
> > > Yes, I thought about that. Some of my thoughts are:
> > >
> > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> > > - What if we run out of flag bits in future?
> > > - We could fold btf_id in the structure in this patchset. And new
> > > fields could be easily added if needed.
> > >
> > > So with these questions, I didn't pursue that approach in the first
> > > place. But I admit that it does look better by writing
> > >
> > > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> > >
> > > Instead of
> > >
> > > +       .arg3    = {
> > > +               .type = ARG_PTR_TO_MAP_VALUE,
> > > +               .flag = ARG_FLAG_MAYBE_NULL,
> > > +       },
> > >
> > > Let's see if there is any further comment. I can go take a look and
> > > prepare for that approach in the next revision.
> > >
> >
> > +1 on staying within a single enum and using upper bits
> >
> > >
> > >
> > > >
> > > > Instead of diff:
> > > > -       .arg1_type      = ARG_CONST_MAP_PTR,
> > > > -       .arg2_type      = ARG_PTR_TO_FUNC,
> > > > -       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > > -       .arg4_type      = ARG_ANYTHING,
> > > > +       .arg1           = { .type = ARG_CONST_MAP_PTR },
> > > > +       .arg2           = { .type = ARG_PTR_TO_FUNC },
> > > > +       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
> > > > +       .arg4           = { .type = ARG_ANYTHING },
> > > >
> > > > can we make it look like:
> > > >        .arg1_type      = ARG_CONST_MAP_PTR,
> > > >        .arg2_type      = ARG_PTR_TO_FUNC,
> > > > -      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> > > >        .arg4_type      = ARG_ANYTHING,
> > > >
> > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> > > > would share the same flag bit: MAYBE_NULL.
> >
> > I support using the same bit value, but should we use the exact same
> > enum name for three different enums? Like MAYBE_NULL, which enum is it
> > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
> > REG_MAYBE_NULL be more explicit about what they apply to?
>
> argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition
> to REG_MAYBE_NULL".

Why differentiate? What difference do you see?
The flag looks the same to me in return, reg and arg.

> >
> > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
> > the same thing, are they? Is the plan to use two different bits for
> > them or pretend that CONST_OR_ZERO "may be null"?

What's the difference ?
I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly
to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM.

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Andrii Nakryiko]

On Tue, Nov 9, 2021 at 8:41 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote:
> > > >
> > > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote:
> > > > > > This is a pure cleanup patchset that tries to use flag to mark whether
> > > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing
> > > > > > so allows us to embed the properties of arguments in the struct, which
> > > > > > is a more scalable solution than introducing a new enum. This patchset
> > > > > > performs this transformation only on arg_type. If it looks good,
> > > > > > follow-up patches will do the same on reg_type and ret_type.
> > > > > >
> > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type'
> > > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs.
> > > > >
> > > > > Nice. Thank you for working on it!
> > > >
> > > > No problem. :)
> > > >
> > > > >
> > > > > The enum->struct conversion works for bpf_arg_type, but applying
> > > > > the same technique to bpf_reg_type could be problematic.
> > > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor.
> > > > > Growing enum from 4 bytes to 8 byte struct will consume quite
> > > > > a lot of extra memory.
> > > > >
> > > > > >  19 files changed, 932 insertions(+), 780 deletions(-)
> > > > >
> > > > > Just bpf_arg_type refactoring adds a lot of churn which could make
> > > > > backports of future fixes not automatic anymore.
> > > > > Similar converstion for bpf_reg_type and bpf_return_type will
> > > > > be even more churn.
> > > >
> > > > Acknowledged.
> > > >
> > > > > Have you considered using upper bits to represent flags?
> > > >
> > > > Yes, I thought about that. Some of my thoughts are:
> > > >
> > > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough?
> > > > - What if we run out of flag bits in future?
> > > > - We could fold btf_id in the structure in this patchset. And new
> > > > fields could be easily added if needed.
> > > >
> > > > So with these questions, I didn't pursue that approach in the first
> > > > place. But I admit that it does look better by writing
> > > >
> > > > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> > > >
> > > > Instead of
> > > >
> > > > +       .arg3    = {
> > > > +               .type = ARG_PTR_TO_MAP_VALUE,
> > > > +               .flag = ARG_FLAG_MAYBE_NULL,
> > > > +       },
> > > >
> > > > Let's see if there is any further comment. I can go take a look and
> > > > prepare for that approach in the next revision.
> > > >
> > >
> > > +1 on staying within a single enum and using upper bits
> > >
> > > >
> > > >
> > > > >
> > > > > Instead of diff:
> > > > > -       .arg1_type      = ARG_CONST_MAP_PTR,
> > > > > -       .arg2_type      = ARG_PTR_TO_FUNC,
> > > > > -       .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > > > -       .arg4_type      = ARG_ANYTHING,
> > > > > +       .arg1           = { .type = ARG_CONST_MAP_PTR },
> > > > > +       .arg2           = { .type = ARG_PTR_TO_FUNC },
> > > > > +       .arg3           = { .type = ARG_PTR_TO_STACK_OR_NULL },
> > > > > +       .arg4           = { .type = ARG_ANYTHING },
> > > > >
> > > > > can we make it look like:
> > > > >        .arg1_type      = ARG_CONST_MAP_PTR,
> > > > >        .arg2_type      = ARG_PTR_TO_FUNC,
> > > > > -      .arg3_type      = ARG_PTR_TO_STACK_OR_NULL,
> > > > > +      .arg3_type      = ARG_PTR_TO_STACK | MAYBE_NULL,
> > > > >        .arg4_type      = ARG_ANYTHING,
> > > > >
> > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type)
> > > > > would share the same flag bit: MAYBE_NULL.
> > >
> > > I support using the same bit value, but should we use the exact same
> > > enum name for three different enums? Like MAYBE_NULL, which enum is it
> > > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to
> > > REG_MAYBE_NULL be more explicit about what they apply to?
> >
> > argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition
> > to REG_MAYBE_NULL".
>
> Why differentiate? What difference do you see?
> The flag looks the same to me in return, reg and arg.

We have three different enums which will be combined with some
constants defined outside of some of those enums. Just have a gut
feeling that this will bite us at some point. Nothing more.

>
> > >
> > > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not
> > > the same thing, are they? Is the plan to use two different bits for
> > > them or pretend that CONST_OR_ZERO "may be null"?
>
> What's the difference ?
> I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly
> to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM.

Again, just gut feeling that this will go wrong.

But also one specific example from kernel/bpf/verifier.c:

if (register_is_null(reg) && arg_type_may_be_null(arg_type))
    goto skip_type_check;

Currently arg_type_may_be_null(arg_type) returns false for
ARG_CONST_SIZE_OR_ZERO. If we are not careful and blindly check the
MAYBE_NULL flag (which the current patch set seems to be doing), we'll
start returning true for it and some other _OR_ZERO arg types. It
might be benign in this particular case, I haven't traced if
ARG_CONST_SIZE_OR_ZERO can be passed in that particular code path, but
it was hardly intended this way, no?

Re: [RFC PATCH bpf-next 0/9] bpf: Clean up _OR_NULL arg types

[Alexei Starovoitov]

On Tue, Nov 09, 2021 at 09:00:25PM -0800, Andrii Nakryiko wrote:
> 
> But also one specific example from kernel/bpf/verifier.c:
> 
> if (register_is_null(reg) && arg_type_may_be_null(arg_type))
>     goto skip_type_check;
> 
> Currently arg_type_may_be_null(arg_type) returns false for
> ARG_CONST_SIZE_OR_ZERO. If we are not careful and blindly check the
> MAYBE_NULL flag (which the current patch set seems to be doing), we'll
> start returning true for it and some other _OR_ZERO arg types. It
> might be benign in this particular case, I haven't traced if
> ARG_CONST_SIZE_OR_ZERO can be passed in that particular code path, but
> it was hardly intended this way, no?

I think it's an example where uniform handling of MAYBE_ZERO flag would have helped.
The case of arg_type_may_be_null() missing in ARG_CONST_SIZE_OR_ZERO doesn't hurt.
The subsequent check_reg_type() is just doing unnecessary work
(it's checking that reg is scalar, but register_is_null already did that).

I think such application of flags would have positive result.
Doesn't mean that we should apply it blindly.
There could be a case where it would be incorrect, but as this example
demonstrated it's more likely to find cases where it's safe to do.
We just forgot to include all of OR_NULL flavors here and could be in other places too.

I think your main point that gotta be very careful about introducing the flags.
That's for sure. As anything that touches the verifier.