diff mbox series

[RFC,bpf-next,v2,1/9] bpf: Introduce composable reg, ret and arg types.

Message ID 20211130012948.380602-2-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 success Errors and warnings before: 12421 this patch: 12421
netdev/cc_maintainers warning 2 maintainers not CCed: netdev@vger.kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 2094 this patch: 2094
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 success Errors and warnings before: 11586 this patch: 11586
netdev/checkpatch warning CHECK: Please use a blank line after function/struct/union/enum declarations CHECK: Prefer using the BIT macro WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 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
There are some common properties shared between bpf reg, ret and arg
values. For instance, a value may be a NULL pointer, or a pointer to
a read-only memory. Previously, to express these properties, enumeration
was used. For example, in order to test whether a reg value can be NULL,
reg_type_may_be_null() simply enumerates all types that are possibly
NULL. The problem of this approach is that it's not scalable and causes
a lot of duplication. These properties can be combined, for example, a
type could be either MAYBE_NULL or RDONLY, or both.

This patch series rewrites the layout of reg_type, arg_type and
ret_type, so that common properties can be extracted and represented as
composable flag. For example, one can write

 ARG_PTR_TO_MEM | PTR_MAYBE_NULL

which is equivalent to the previous

 ARG_PTR_TO_MEM_OR_NULL

The type ARG_PTR_TO_MEM are called "base types" in this patch. Base
types can be extended with flags. A flag occupies the higher bits while
base types sits in the lower bits.

This patch in particular sets up a set of macro for this purpose. The
followed patches rewrites arg_types, ret_types and reg_types
respectively.

Signed-off-by: Hao Luo <haoluo@google.com>
---
 include/linux/bpf.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Alexei Starovoitov Dec. 1, 2021, 8:29 p.m. UTC | #1
On Mon, Nov 29, 2021 at 05:29:40PM -0800, Hao Luo wrote:
> There are some common properties shared between bpf reg, ret and arg
> values. For instance, a value may be a NULL pointer, or a pointer to
> a read-only memory. Previously, to express these properties, enumeration
> was used. For example, in order to test whether a reg value can be NULL,
> reg_type_may_be_null() simply enumerates all types that are possibly
> NULL. The problem of this approach is that it's not scalable and causes
> a lot of duplication. These properties can be combined, for example, a
> type could be either MAYBE_NULL or RDONLY, or both.
> 
> This patch series rewrites the layout of reg_type, arg_type and
> ret_type, so that common properties can be extracted and represented as
> composable flag. For example, one can write
> 
>  ARG_PTR_TO_MEM | PTR_MAYBE_NULL
> 
> which is equivalent to the previous
> 
>  ARG_PTR_TO_MEM_OR_NULL
> 
> The type ARG_PTR_TO_MEM are called "base types" in this patch. Base
> types can be extended with flags. A flag occupies the higher bits while
> base types sits in the lower bits.
> 
> This patch in particular sets up a set of macro for this purpose. The
> followed patches rewrites arg_types, ret_types and reg_types
> respectively.
> 
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---
>  include/linux/bpf.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cc7a0c36e7df..b592b3f7d223 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -297,6 +297,37 @@ bool bpf_map_meta_equal(const struct bpf_map *meta0,
>  
>  extern const struct bpf_map_ops bpf_map_offload_ops;
>  
> +/* bpf_type_flag contains a set of flags that are applicable to the values of
> + * arg_type, ret_type and reg_type. For example, a pointer value may be null,
> + * or a memory is read-only. We classify types into two categories: base types
> + * and extended types. Extended types are base types combined with a type flag.
> + *
> + * Currently there are no more than 32 base types in arg_type, ret_type and
> + * reg_types.
> + */
> +#define BPF_BASE_TYPE_BITS	8
> +
> +enum bpf_type_flag {
> +	/* PTR may be NULL. */
> +	PTR_MAYBE_NULL		= BIT(0 + BPF_BASE_TYPE_BITS),
> +
> +	__BPF_TYPE_LAST_FLAG	= PTR_MAYBE_NULL,
> +};
> +
> +#define BPF_BASE_TYPE_MASK	GENMASK(BPF_BASE_TYPE_BITS, 0)
> +
> +/* Max number of base types. */
> +#define BPF_BASE_TYPE_LIMIT	(1UL << BPF_BASE_TYPE_BITS)
> +
> +/* Max number of all types. */
> +#define BPF_TYPE_LIMIT		(__BPF_TYPE_LAST_FLAG | (__BPF_TYPE_LAST_FLAG - 1))
> +
> +/* extract base type. */
> +#define BPF_BASE_TYPE(x)	((x) & BPF_BASE_TYPE_MASK)
> +
> +/* extract flags from an extended type. */
> +#define BPF_TYPE_FLAG(x)	((enum bpf_type_flag)((x) & ~BPF_BASE_TYPE_MASK))

Overall I think it's really great.
The only suggestion is to use:
static inline u32 base_type(u32 x)
{
  return x & BPF_BASE_TYPE_MASK;
}
and
static inline u32 type_flag(u32 x) ..

The capital letter macros are too loud.

wdyt?
Hao Luo Dec. 1, 2021, 10:36 p.m. UTC | #2
On Wed, Dec 1, 2021 at 12:29 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Nov 29, 2021 at 05:29:40PM -0800, Hao Luo wrote:
> > There are some common properties shared between bpf reg, ret and arg
> > values. For instance, a value may be a NULL pointer, or a pointer to
> > a read-only memory. Previously, to express these properties, enumeration
> > was used. For example, in order to test whether a reg value can be NULL,
> > reg_type_may_be_null() simply enumerates all types that are possibly
> > NULL. The problem of this approach is that it's not scalable and causes
> > a lot of duplication. These properties can be combined, for example, a
> > type could be either MAYBE_NULL or RDONLY, or both.
> >
> > This patch series rewrites the layout of reg_type, arg_type and
> > ret_type, so that common properties can be extracted and represented as
> > composable flag. For example, one can write
> >
> >  ARG_PTR_TO_MEM | PTR_MAYBE_NULL
> >
> > which is equivalent to the previous
> >
> >  ARG_PTR_TO_MEM_OR_NULL
> >
> > The type ARG_PTR_TO_MEM are called "base types" in this patch. Base
> > types can be extended with flags. A flag occupies the higher bits while
> > base types sits in the lower bits.
> >
> > This patch in particular sets up a set of macro for this purpose. The
> > followed patches rewrites arg_types, ret_types and reg_types
> > respectively.
> >
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> >  include/linux/bpf.h | 50 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index cc7a0c36e7df..b592b3f7d223 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -297,6 +297,37 @@ bool bpf_map_meta_equal(const struct bpf_map *meta0,
> >
> >  extern const struct bpf_map_ops bpf_map_offload_ops;
> >
> > +/* bpf_type_flag contains a set of flags that are applicable to the values of
> > + * arg_type, ret_type and reg_type. For example, a pointer value may be null,
> > + * or a memory is read-only. We classify types into two categories: base types
> > + * and extended types. Extended types are base types combined with a type flag.
> > + *
> > + * Currently there are no more than 32 base types in arg_type, ret_type and
> > + * reg_types.
> > + */
> > +#define BPF_BASE_TYPE_BITS   8
> > +
> > +enum bpf_type_flag {
> > +     /* PTR may be NULL. */
> > +     PTR_MAYBE_NULL          = BIT(0 + BPF_BASE_TYPE_BITS),
> > +
> > +     __BPF_TYPE_LAST_FLAG    = PTR_MAYBE_NULL,
> > +};
> > +
> > +#define BPF_BASE_TYPE_MASK   GENMASK(BPF_BASE_TYPE_BITS, 0)
> > +
> > +/* Max number of base types. */
> > +#define BPF_BASE_TYPE_LIMIT  (1UL << BPF_BASE_TYPE_BITS)
> > +
> > +/* Max number of all types. */
> > +#define BPF_TYPE_LIMIT               (__BPF_TYPE_LAST_FLAG | (__BPF_TYPE_LAST_FLAG - 1))
> > +
> > +/* extract base type. */
> > +#define BPF_BASE_TYPE(x)     ((x) & BPF_BASE_TYPE_MASK)
> > +
> > +/* extract flags from an extended type. */
> > +#define BPF_TYPE_FLAG(x)     ((enum bpf_type_flag)((x) & ~BPF_BASE_TYPE_MASK))
>
> Overall I think it's really great.
> The only suggestion is to use:
> static inline u32 base_type(u32 x)
> {
>   return x & BPF_BASE_TYPE_MASK;
> }
> and
> static inline u32 type_flag(u32 x) ..
>
> The capital letter macros are too loud.
>
> wdyt?
>

Sounds good to me. Will do in the next iteration.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cc7a0c36e7df..b592b3f7d223 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -297,6 +297,37 @@  bool bpf_map_meta_equal(const struct bpf_map *meta0,
 
 extern const struct bpf_map_ops bpf_map_offload_ops;
 
+/* bpf_type_flag contains a set of flags that are applicable to the values of
+ * arg_type, ret_type and reg_type. For example, a pointer value may be null,
+ * or a memory is read-only. We classify types into two categories: base types
+ * and extended types. Extended types are base types combined with a type flag.
+ *
+ * Currently there are no more than 32 base types in arg_type, ret_type and
+ * reg_types.
+ */
+#define BPF_BASE_TYPE_BITS	8
+
+enum bpf_type_flag {
+	/* PTR may be NULL. */
+	PTR_MAYBE_NULL		= BIT(0 + BPF_BASE_TYPE_BITS),
+
+	__BPF_TYPE_LAST_FLAG	= PTR_MAYBE_NULL,
+};
+
+#define BPF_BASE_TYPE_MASK	GENMASK(BPF_BASE_TYPE_BITS, 0)
+
+/* Max number of base types. */
+#define BPF_BASE_TYPE_LIMIT	(1UL << BPF_BASE_TYPE_BITS)
+
+/* Max number of all types. */
+#define BPF_TYPE_LIMIT		(__BPF_TYPE_LAST_FLAG | (__BPF_TYPE_LAST_FLAG - 1))
+
+/* extract base type. */
+#define BPF_BASE_TYPE(x)	((x) & BPF_BASE_TYPE_MASK)
+
+/* extract flags from an extended type. */
+#define BPF_TYPE_FLAG(x)	((enum bpf_type_flag)((x) & ~BPF_BASE_TYPE_MASK))
+
 /* function argument constraints */
 enum bpf_arg_type {
 	ARG_DONTCARE = 0,	/* unused argument in helper function */
@@ -343,7 +374,13 @@  enum bpf_arg_type {
 	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,
+
+	/* 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.
+	 */
+	__BPF_ARG_TYPE_LIMIT	= BPF_TYPE_LIMIT,
 };
+static_assert(__BPF_ARG_TYPE_MAX <= BPF_BASE_TYPE_LIMIT);
 
 /* type of values returned from helper functions */
 enum bpf_return_type {
@@ -359,7 +396,14 @@  enum bpf_return_type {
 	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_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,
+
+	/* 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.
+	 */
+	__BPF_RET_TYPE_LIMIT	= BPF_TYPE_LIMIT,
 };
+static_assert(__BPF_RET_TYPE_MAX <= BPF_BASE_TYPE_LIMIT);
 
 /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
  * to in-kernel helper functions and for adjusting imm32 field in BPF_CALL
@@ -461,7 +505,13 @@  enum bpf_reg_type {
 	PTR_TO_FUNC,		 /* reg points to a bpf program function */
 	PTR_TO_MAP_KEY,		 /* reg points to a map element key */
 	__BPF_REG_TYPE_MAX,
+
+	/* 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.
+	 */
+	__BPF_REG_TYPE_LIMIT	= BPF_TYPE_LIMIT,
 };
+static_assert(__BPF_REG_TYPE_MAX <= BPF_BASE_TYPE_LIMIT);
 
 /* The information passed from prog-specific *_is_valid_access
  * back to the verifier.