[bpf-next,v1,4/7] bpf: Add bpf_dynptr_read and bpf_dynptr_write

Commit Message

Joanne Koong April 2, 2022, 1:58 a.m. UTC

From: Joanne Koong <[e-mail]>

This patch adds two helper functions, bpf_dynptr_read and
bpf_dynptr_write:

long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);

long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);

The dynptr passed into these functions must be valid dynptrs that have
been initialized.

Signed-off-by: Joanne Koong <[e-mail]>
---
 include/linux/bpf.h            |  6 ++++
 include/uapi/linux/bpf.h       | 18 +++++++++++
 kernel/bpf/helpers.c           | 56 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 18 +++++++++++
 4 files changed, 98 insertions(+)

Comments

Toke Høiland-Jørgensen April 2, 2022, 1:35 p.m. UTC | #1

Joanne Koong <[e-mail]> writes:

> From: Joanne Koong <[e-mail]>
>
> This patch adds two helper functions, bpf_dynptr_read and
> bpf_dynptr_write:
>
> long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
>
> long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
>
> The dynptr passed into these functions must be valid dynptrs that have
> been initialized.
>
> Signed-off-by: Joanne Koong <[e-mail]>
> ---
>  include/linux/bpf.h            |  6 ++++
>  include/uapi/linux/bpf.h       | 18 +++++++++++
>  kernel/bpf/helpers.c           | 56 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 18 +++++++++++
>  4 files changed, 98 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e0fcff9f2aee..cded9753fb7f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2426,6 +2426,12 @@ enum bpf_dynptr_type {
>  #define DYNPTR_MAX_SIZE	((1UL << 28) - 1)
>  #define DYNPTR_SIZE_MASK	0xFFFFFFF
>  #define DYNPTR_TYPE_SHIFT	29
> +#define DYNPTR_RDONLY_BIT	BIT(28)
> +
> +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> +{
> +	return ptr->size & DYNPTR_RDONLY_BIT;
> +}
>  
>  static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
>  {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 6a57d8a1b882..16a35e46be90 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5175,6 +5175,22 @@ union bpf_attr {
>   *		After this operation, *ptr* will be an invalidated dynptr.
>   *	Return
>   *		Void.
> + *
> + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> + *	Description
> + *		Read *len* bytes from *src* into *dst*, starting from *offset*
> + *		into *dst*.

nit: this should be "starting from *offset* into *src*, no? (same below)

-Toke

Joanne Koong April 4, 2022, 8:18 p.m. UTC | #2

On Sat, Apr 2, 2022 at 6:35 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Joanne Koong <[e-mail]> writes:
>
> > From: Joanne Koong <[e-mail]>
> >
> > This patch adds two helper functions, bpf_dynptr_read and
> > bpf_dynptr_write:
> >
> > long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
> >
> > long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
> >
> > The dynptr passed into these functions must be valid dynptrs that have
> > been initialized.
> >
> > Signed-off-by: Joanne Koong <[e-mail]>
> > ---
> >  include/linux/bpf.h            |  6 ++++
> >  include/uapi/linux/bpf.h       | 18 +++++++++++
> >  kernel/bpf/helpers.c           | 56 ++++++++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h | 18 +++++++++++
> >  4 files changed, 98 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index e0fcff9f2aee..cded9753fb7f 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2426,6 +2426,12 @@ enum bpf_dynptr_type {
> >  #define DYNPTR_MAX_SIZE      ((1UL << 28) - 1)
> >  #define DYNPTR_SIZE_MASK     0xFFFFFFF
> >  #define DYNPTR_TYPE_SHIFT    29
> > +#define DYNPTR_RDONLY_BIT    BIT(28)
> > +
> > +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> > +{
> > +     return ptr->size & DYNPTR_RDONLY_BIT;
> > +}
> >
> >  static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
> >  {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 6a57d8a1b882..16a35e46be90 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5175,6 +5175,22 @@ union bpf_attr {
> >   *           After this operation, *ptr* will be an invalidated dynptr.
> >   *   Return
> >   *           Void.
> > + *
> > + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> > + *   Description
> > + *           Read *len* bytes from *src* into *dst*, starting from *offset*
> > + *           into *dst*.
>
> nit: this should be "starting from *offset* into *src*, no? (same below)
>
Yes, this should be "starting from *offset* into *src*". I will fix
this line in both places. Thanks!
> -Toke
>

Andrii Nakryiko April 6, 2022, 10:32 p.m. UTC | #3

On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <[e-mail]> wrote:
>
> From: Joanne Koong <[e-mail]>
>
> This patch adds two helper functions, bpf_dynptr_read and
> bpf_dynptr_write:
>
> long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
>
> long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
>
> The dynptr passed into these functions must be valid dynptrs that have
> been initialized.
>
> Signed-off-by: Joanne Koong <[e-mail]>
> ---
>  include/linux/bpf.h            |  6 ++++
>  include/uapi/linux/bpf.h       | 18 +++++++++++
>  kernel/bpf/helpers.c           | 56 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 18 +++++++++++
>  4 files changed, 98 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e0fcff9f2aee..cded9753fb7f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2426,6 +2426,12 @@ enum bpf_dynptr_type {
>  #define DYNPTR_MAX_SIZE        ((1UL << 28) - 1)
>  #define DYNPTR_SIZE_MASK       0xFFFFFFF
>  #define DYNPTR_TYPE_SHIFT      29
> +#define DYNPTR_RDONLY_BIT      BIT(28)
> +
> +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> +{
> +       return ptr->size & DYNPTR_RDONLY_BIT;
> +}
>
>  static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
>  {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 6a57d8a1b882..16a35e46be90 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5175,6 +5175,22 @@ union bpf_attr {
>   *             After this operation, *ptr* will be an invalidated dynptr.
>   *     Return
>   *             Void.
> + *
> + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> + *     Description
> + *             Read *len* bytes from *src* into *dst*, starting from *offset*
> + *             into *dst*.
> + *     Return
> + *             0 on success, -EINVAL if *offset* + *len* exceeds the length
> + *             of *src*'s data or if *src* is an invalid dynptr.
> + *
> + * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
> + *     Description
> + *             Write *len* bytes from *src* into *dst*, starting from *offset*
> + *             into *dst*.
> + *     Return
> + *             0 on success, -EINVAL if *offset* + *len* exceeds the length
> + *             of *dst*'s data or if *dst* is not writeable.

Did you plan to also add a helper to copy from one dynptr to another?
Something like

long bpf_dynptr_copy(struct bpf_dynptr *dst, struct bpf_dyn_ptr *src, u32 len) ?

Otherwise there won't be any way to copy memory from malloc'ed range
to ringbuf, for example, without doing intermediate copy. Not sure
what to do about extra offsets...

>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -5374,6 +5390,8 @@ union bpf_attr {
>         FN(dynptr_from_mem),            \
>         FN(malloc),                     \
>         FN(free),                       \
> +       FN(dynptr_read),                \
> +       FN(dynptr_write),               \
>         /* */
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index ed5a7d9d0a18..7ec20e79928e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1412,6 +1412,58 @@ const struct bpf_func_proto bpf_dynptr_from_mem_proto = {
>         .arg3_type      = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT,
>  };
>
> +BPF_CALL_4(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, u32, offset)
> +{
> +       int err;
> +
> +       if (!src->data)
> +               return -EINVAL;
> +
> +       err = bpf_dynptr_check_off_len(src, offset, len);

you defined this function in patch #3, but didn't use it there. Let's
move the definition into this patch?

> +       if (err)
> +               return err;
> +
> +       memcpy(dst, src->data + src->offset + offset, len);
> +
> +       return 0;
> +}
> +

[...]

Joanne Koong April 8, 2022, 11:07 p.m. UTC | #4

On Wed, Apr 6, 2022 at 3:32 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <[e-mail]> wrote:
> >
> > From: Joanne Koong <[e-mail]>
> >
> > This patch adds two helper functions, bpf_dynptr_read and
> > bpf_dynptr_write:
> >
> > long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
> >
> > long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
> >
> > The dynptr passed into these functions must be valid dynptrs that have
> > been initialized.
> >
> > Signed-off-by: Joanne Koong <[e-mail]>
> > ---
> >  include/linux/bpf.h            |  6 ++++
> >  include/uapi/linux/bpf.h       | 18 +++++++++++
> >  kernel/bpf/helpers.c           | 56 ++++++++++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h | 18 +++++++++++
> >  4 files changed, 98 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index e0fcff9f2aee..cded9753fb7f 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2426,6 +2426,12 @@ enum bpf_dynptr_type {
> >  #define DYNPTR_MAX_SIZE        ((1UL << 28) - 1)
> >  #define DYNPTR_SIZE_MASK       0xFFFFFFF
> >  #define DYNPTR_TYPE_SHIFT      29
> > +#define DYNPTR_RDONLY_BIT      BIT(28)
> > +
> > +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> > +{
> > +       return ptr->size & DYNPTR_RDONLY_BIT;
> > +}
> >
> >  static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
> >  {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 6a57d8a1b882..16a35e46be90 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5175,6 +5175,22 @@ union bpf_attr {
> >   *             After this operation, *ptr* will be an invalidated dynptr.
> >   *     Return
> >   *             Void.
> > + *
> > + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> > + *     Description
> > + *             Read *len* bytes from *src* into *dst*, starting from *offset*
> > + *             into *dst*.
> > + *     Return
> > + *             0 on success, -EINVAL if *offset* + *len* exceeds the length
> > + *             of *src*'s data or if *src* is an invalid dynptr.
> > + *
> > + * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
> > + *     Description
> > + *             Write *len* bytes from *src* into *dst*, starting from *offset*
> > + *             into *dst*.
> > + *     Return
> > + *             0 on success, -EINVAL if *offset* + *len* exceeds the length
> > + *             of *dst*'s data or if *dst* is not writeable.
>
> Did you plan to also add a helper to copy from one dynptr to another?
> Something like
>
> long bpf_dynptr_copy(struct bpf_dynptr *dst, struct bpf_dyn_ptr *src, u32 len) ?
>
> Otherwise there won't be any way to copy memory from malloc'ed range
> to ringbuf, for example, without doing intermediate copy. Not sure
> what to do about extra offsets...
Yes! I plan for the 3rd patchset in this dynptr series to be around
convenience helpers, which will include bpf_dynptr_copy.
For the offsets, I was thinking just copy from src data + src internal
offset to dst data + dst internal offset, where there will also be
dynptr helper functions that can be called to adjust offsets
>
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)          \
> >         FN(unspec),                     \
> > @@ -5374,6 +5390,8 @@ union bpf_attr {
> >         FN(dynptr_from_mem),            \
> >         FN(malloc),                     \
> >         FN(free),                       \
> > +       FN(dynptr_read),                \
> > +       FN(dynptr_write),               \
> >         /* */
> >
> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index ed5a7d9d0a18..7ec20e79928e 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1412,6 +1412,58 @@ const struct bpf_func_proto bpf_dynptr_from_mem_proto = {
> >         .arg3_type      = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT,
> >  };
> >
> > +BPF_CALL_4(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, u32, offset)
> > +{
> > +       int err;
> > +
> > +       if (!src->data)
> > +               return -EINVAL;
> > +
> > +       err = bpf_dynptr_check_off_len(src, offset, len);
>
> you defined this function in patch #3, but didn't use it there. Let's
> move the definition into this patch?
Sounds great!
>
> > +       if (err)
> > +               return err;
> > +
> > +       memcpy(dst, src->data + src->offset + offset, len);
> > +
> > +       return 0;
> > +}
> > +
>
> [...]

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e0fcff9f2aee..cded9753fb7f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2426,6 +2426,12 @@  enum bpf_dynptr_type {
 #define DYNPTR_MAX_SIZE	((1UL << 28) - 1)
 #define DYNPTR_SIZE_MASK	0xFFFFFFF
 #define DYNPTR_TYPE_SHIFT	29
+#define DYNPTR_RDONLY_BIT	BIT(28)
+
+static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
+{
+	return ptr->size & DYNPTR_RDONLY_BIT;
+}
 
 static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
 {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6a57d8a1b882..16a35e46be90 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5175,6 +5175,22 @@  union bpf_attr {
  *		After this operation, *ptr* will be an invalidated dynptr.
  *	Return
  *		Void.
+ *
+ * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
+ *	Description
+ *		Read *len* bytes from *src* into *dst*, starting from *offset*
+ *		into *dst*.
+ *	Return
+ *		0 on success, -EINVAL if *offset* + *len* exceeds the length
+ *		of *src*'s data or if *src* is an invalid dynptr.
+ *
+ * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
+ *	Description
+ *		Write *len* bytes from *src* into *dst*, starting from *offset*
+ *		into *dst*.
+ *	Return
+ *		0 on success, -EINVAL if *offset* + *len* exceeds the length
+ *		of *dst*'s data or if *dst* is not writeable.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5374,6 +5390,8 @@  union bpf_attr {
 	FN(dynptr_from_mem),		\
 	FN(malloc),			\
 	FN(free),			\
+	FN(dynptr_read),		\
+	FN(dynptr_write),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index ed5a7d9d0a18..7ec20e79928e 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1412,6 +1412,58 @@  const struct bpf_func_proto bpf_dynptr_from_mem_proto = {
 	.arg3_type	= ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT,
 };
 
+BPF_CALL_4(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, u32, offset)
+{
+	int err;
+
+	if (!src->data)
+		return -EINVAL;
+
+	err = bpf_dynptr_check_off_len(src, offset, len);
+	if (err)
+		return err;
+
+	memcpy(dst, src->data + src->offset + offset, len);
+
+	return 0;
+}
+
+const struct bpf_func_proto bpf_dynptr_read_proto = {
+	.func		= bpf_dynptr_read,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_MEM_UNINIT,
+	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
+	.arg3_type	= ARG_PTR_TO_DYNPTR,
+	.arg4_type	= ARG_ANYTHING,
+};
+
+BPF_CALL_4(bpf_dynptr_write, struct bpf_dynptr_kern *, dst, u32, offset, void *, src, u32, len)
+{
+	int err;
+
+	if (!dst->data || bpf_dynptr_is_rdonly(dst))
+		return -EINVAL;
+
+	err = bpf_dynptr_check_off_len(dst, offset, len);
+	if (err)
+		return err;
+
+	memcpy(dst->data + dst->offset + offset, src, len);
+
+	return 0;
+}
+
+const struct bpf_func_proto bpf_dynptr_write_proto = {
+	.func		= bpf_dynptr_write,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_DYNPTR,
+	.arg2_type	= ARG_ANYTHING,
+	.arg3_type	= ARG_PTR_TO_MEM | MEM_RDONLY,
+	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
+};
+
 BPF_CALL_2(bpf_malloc, u32, size, struct bpf_dynptr_kern *, ptr)
 {
 	void *data;
@@ -1514,6 +1566,10 @@  bpf_base_func_proto(enum bpf_func_id func_id)
 		return &bpf_malloc_proto;
 	case BPF_FUNC_free:
 		return &bpf_free_proto;
+	case BPF_FUNC_dynptr_read:
+		return &bpf_dynptr_read_proto;
+	case BPF_FUNC_dynptr_write:
+		return &bpf_dynptr_write_proto;
 	default:
 		break;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6a57d8a1b882..16a35e46be90 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5175,6 +5175,22 @@  union bpf_attr {
  *		After this operation, *ptr* will be an invalidated dynptr.
  *	Return
  *		Void.
+ *
+ * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
+ *	Description
+ *		Read *len* bytes from *src* into *dst*, starting from *offset*
+ *		into *dst*.
+ *	Return
+ *		0 on success, -EINVAL if *offset* + *len* exceeds the length
+ *		of *src*'s data or if *src* is an invalid dynptr.
+ *
+ * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
+ *	Description
+ *		Write *len* bytes from *src* into *dst*, starting from *offset*
+ *		into *dst*.
+ *	Return
+ *		0 on success, -EINVAL if *offset* + *len* exceeds the length
+ *		of *dst*'s data or if *dst* is not writeable.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5374,6 +5390,8 @@  union bpf_attr {
 	FN(dynptr_from_mem),		\
 	FN(malloc),			\
 	FN(free),			\
+	FN(dynptr_read),		\
+	FN(dynptr_write),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper