diff mbox series

[bpf-next,v6,05/10] bpf: support attaching freplace programs to multiple attach points

Message ID 160037400605.28970.12030576233071570541.stgit@toke.dk
State New
Headers show
Series bpf: Support multi-attach for freplace programs | expand

Commit Message

Toke Høiland-Jørgensen Sept. 17, 2020, 8:20 p.m. UTC 
From: Toke Høiland-Jørgensen <toke@redhat.com>

This enables support for attaching freplace programs to multiple attach
points. It does this by amending the UAPI for bpf_link_Create with a target
btf ID that can be used to supply the new attachment point along with the
target program fd. The target must be compatible with the target that was
supplied at program load time.

The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.

The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, there is no API support for doing so.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h            |    2 +
 include/uapi/linux/bpf.h       |    9 +++-
 kernel/bpf/syscall.c           |  101 ++++++++++++++++++++++++++++++++++------
 kernel/bpf/verifier.c          |    9 ++++
 tools/include/uapi/linux/bpf.h |    9 +++-
 5 files changed, 110 insertions(+), 20 deletions(-)

Comments

Andrii Nakryiko Sept. 18, 2020, 6:47 p.m. UTC | #1 
On Thu, Sep 17, 2020 at 1:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This enables support for attaching freplace programs to multiple attach
> points. It does this by amending the UAPI for bpf_link_Create with a target
> btf ID that can be used to supply the new attachment point along with the
> target program fd. The target must be compatible with the target that was
> supplied at program load time.
>
> The implementation reuses the checks that were factored out of
> check_attach_btf_id() to ensure compatibility between the BTF types of the
> old and new attachment. If these match, a new bpf_tracing_link will be
> created for the new attach target, allowing multiple attachments to
> co-exist simultaneously.
>
> The code could theoretically support multiple-attach of other types of
> tracing programs as well, but since I don't have a use case for any of
> those, there is no API support for doing so.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

You patch set breaks at least bpf_iter tests:

$ sudo ./test_progs -t bpf_iter
...
#4 bpf_iter:FAIL
Summary: 0/19 PASSED, 0 SKIPPED, 6 FAILED

Please check and fix.

>  include/linux/bpf.h            |    2 +
>  include/uapi/linux/bpf.h       |    9 +++-
>  kernel/bpf/syscall.c           |  101 ++++++++++++++++++++++++++++++++++------
>  kernel/bpf/verifier.c          |    9 ++++
>  tools/include/uapi/linux/bpf.h |    9 +++-
>  5 files changed, 110 insertions(+), 20 deletions(-)
>

[...]

> -static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> +static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> +                                  int tgt_prog_fd,
> +                                  u32 btf_id)
>  {
>         struct bpf_link_primer link_primer;
>         struct bpf_prog *tgt_prog = NULL;
> +       struct bpf_trampoline *tr = NULL;
>         struct bpf_tracing_link *link;
> -       struct bpf_trampoline *tr;
> +       struct btf_func_model fmodel;
> +       u64 key = 0;
> +       long addr;
>         int err;
>
>         switch (prog->type) {
> @@ -2589,6 +2595,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)

bpf_tracing_prog_attach logic looks correct to me now, thanks.

>                 goto out_put_prog;
>         }
>

[...]

> @@ -3934,6 +3986,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
>         return -EINVAL;
>  }
>
> +static int freplace_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)

Any reason to have this separate from tracing_bpf_link_attach? I'd
merge them and do a simple switch inside, based on prog->type. It
would also be easier to follow the flow if this expected_attach_type
check was first and returned -EINVAL immediately at the top.


> +{
> +       if (attr->link_create.attach_type == prog->expected_attach_type)
> +               return bpf_tracing_prog_attach(prog,
> +                                              attr->link_create.target_fd,
> +                                              attr->link_create.target_btf_id);
> +       return -EINVAL;
> +

nit: unnecessary empty line?

> +}
> +
>  #define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
>  static int link_create(union bpf_attr *attr)
>  {
> @@ -3944,18 +4006,25 @@ static int link_create(union bpf_attr *attr)
>         if (CHECK_ATTR(BPF_LINK_CREATE))
>                 return -EINVAL;
>
> -       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
> -       if (ptype == BPF_PROG_TYPE_UNSPEC)
> -               return -EINVAL;
> -
> -       prog = bpf_prog_get_type(attr->link_create.prog_fd, ptype);
> +       prog = bpf_prog_get(attr->link_create.prog_fd);
>         if (IS_ERR(prog))
>                 return PTR_ERR(prog);
>
>         ret = bpf_prog_attach_check_attach_type(prog,
>                                                 attr->link_create.attach_type);
>         if (ret)
> -               goto err_out;
> +               goto out;
> +
> +       if (prog->type == BPF_PROG_TYPE_EXT) {
> +               ret = freplace_bpf_link_attach(attr, prog);
> +               goto out;
> +       }
> +
> +       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
> +       if (ptype == BPF_PROG_TYPE_UNSPEC) {
> +               ret = -EINVAL;
> +               goto out;
> +       }

you seem to be missing a check that prog->type matches ptype,
previously implicitly performed by bpf_prog_get_type(), no?

>
>         switch (ptype) {
>         case BPF_PROG_TYPE_CGROUP_SKB:
> @@ -3983,7 +4052,7 @@ static int link_create(union bpf_attr *attr)
>                 ret = -EINVAL;
>         }
>
> -err_out:
> +out:
>         if (ret < 0)
>                 bpf_prog_put(prog);
>         return ret;

[...]
Toke Høiland-Jørgensen Sept. 19, 2020, 10:14 a.m. UTC | #2 
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 17, 2020 at 1:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> This enables support for attaching freplace programs to multiple attach
>> points. It does this by amending the UAPI for bpf_link_Create with a target
>> btf ID that can be used to supply the new attachment point along with the
>> target program fd. The target must be compatible with the target that was
>> supplied at program load time.
>>
>> The implementation reuses the checks that were factored out of
>> check_attach_btf_id() to ensure compatibility between the BTF types of the
>> old and new attachment. If these match, a new bpf_tracing_link will be
>> created for the new attach target, allowing multiple attachments to
>> co-exist simultaneously.
>>
>> The code could theoretically support multiple-attach of other types of
>> tracing programs as well, but since I don't have a use case for any of
>> those, there is no API support for doing so.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> You patch set breaks at least bpf_iter tests:
>
> $ sudo ./test_progs -t bpf_iter
> ...
> #4 bpf_iter:FAIL
> Summary: 0/19 PASSED, 0 SKIPPED, 6 FAILED
>
> Please check and fix.

Huh, did notice something was broken, but they didn't when I reverted
the patch either, so I put it down to just the tests being broken. I'll
take another look :)

>>  include/linux/bpf.h            |    2 +
>>  include/uapi/linux/bpf.h       |    9 +++-
>>  kernel/bpf/syscall.c           |  101 ++++++++++++++++++++++++++++++++++------
>>  kernel/bpf/verifier.c          |    9 ++++
>>  tools/include/uapi/linux/bpf.h |    9 +++-
>>  5 files changed, 110 insertions(+), 20 deletions(-)
>>
>
> [...]
>
>> -static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>> +static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>> +                                  int tgt_prog_fd,
>> +                                  u32 btf_id)
>>  {
>>         struct bpf_link_primer link_primer;
>>         struct bpf_prog *tgt_prog = NULL;
>> +       struct bpf_trampoline *tr = NULL;
>>         struct bpf_tracing_link *link;
>> -       struct bpf_trampoline *tr;
>> +       struct btf_func_model fmodel;
>> +       u64 key = 0;
>> +       long addr;
>>         int err;
>>
>>         switch (prog->type) {
>> @@ -2589,6 +2595,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>
> bpf_tracing_prog_attach logic looks correct to me now, thanks.
>
>>                 goto out_put_prog;
>>         }
>>
>
> [...]
>
>> @@ -3934,6 +3986,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
>>         return -EINVAL;
>>  }
>>
>> +static int freplace_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>
> Any reason to have this separate from tracing_bpf_link_attach? I'd
> merge them and do a simple switch inside, based on prog->type. It
> would also be easier to follow the flow if this expected_attach_type
> check was first and returned -EINVAL immediately at the top.

I created a different one function it had to be called at a different
place; don't mind combining them, though.

>> +{
>> +       if (attr->link_create.attach_type == prog->expected_attach_type)
>> +               return bpf_tracing_prog_attach(prog,
>> +                                              attr->link_create.target_fd,
>> +                                              attr->link_create.target_btf_id);
>> +       return -EINVAL;
>> +
>
> nit: unnecessary empty line?
>
>> +}
>> +
>>  #define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
>>  static int link_create(union bpf_attr *attr)
>>  {
>> @@ -3944,18 +4006,25 @@ static int link_create(union bpf_attr *attr)
>>         if (CHECK_ATTR(BPF_LINK_CREATE))
>>                 return -EINVAL;
>>
>> -       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
>> -       if (ptype == BPF_PROG_TYPE_UNSPEC)
>> -               return -EINVAL;
>> -
>> -       prog = bpf_prog_get_type(attr->link_create.prog_fd, ptype);
>> +       prog = bpf_prog_get(attr->link_create.prog_fd);
>>         if (IS_ERR(prog))
>>                 return PTR_ERR(prog);
>>
>>         ret = bpf_prog_attach_check_attach_type(prog,
>>                                                 attr->link_create.attach_type);
>>         if (ret)
>> -               goto err_out;
>> +               goto out;
>> +
>> +       if (prog->type == BPF_PROG_TYPE_EXT) {
>> +               ret = freplace_bpf_link_attach(attr, prog);
>> +               goto out;
>> +       }
>> +
>> +       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
>> +       if (ptype == BPF_PROG_TYPE_UNSPEC) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>
> you seem to be missing a check that prog->type matches ptype,
> previously implicitly performed by bpf_prog_get_type(), no?

Ah yes, good catch! I played around with different versions of this, and
guess I forgot to put that check back in for this one...

-Toke
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9dbac21c8091..094156d3dd9d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -743,6 +743,8 @@  struct bpf_prog_aux {
 	struct mutex tgt_mutex; /* protects writing of tgt_* pointers below */
 	struct bpf_prog *tgt_prog;
 	struct bpf_trampoline *tgt_trampoline;
+	enum bpf_prog_type tgt_prog_type;
+	enum bpf_attach_type tgt_attach_type;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a22812561064..feff1ed49f86 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -632,8 +632,13 @@  union bpf_attr {
 		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
-		__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
-		__u32		iter_info_len;	/* iter_info length */
+		union {
+			__u32		target_btf_id;	/* btf_id of target to attach to */
+			struct {
+				__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
+				__u32		iter_info_len;	/* iter_info length */
+			};
+		};
 	} link_create;
 
 	struct { /* struct used by BPF_LINK_UPDATE command */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4af35a59d0d9..9d542f1934f2 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4,6 +4,7 @@ 
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/bpf_lirc.h>
+#include <linux/bpf_verifier.h>
 #include <linux/btf.h>
 #include <linux/syscalls.h>
 #include <linux/slab.h>
@@ -2555,12 +2556,17 @@  static const struct bpf_link_ops bpf_tracing_link_lops = {
 	.fill_link_info = bpf_tracing_link_fill_link_info,
 };
 
-static int bpf_tracing_prog_attach(struct bpf_prog *prog)
+static int bpf_tracing_prog_attach(struct bpf_prog *prog,
+				   int tgt_prog_fd,
+				   u32 btf_id)
 {
 	struct bpf_link_primer link_primer;
 	struct bpf_prog *tgt_prog = NULL;
+	struct bpf_trampoline *tr = NULL;
 	struct bpf_tracing_link *link;
-	struct bpf_trampoline *tr;
+	struct btf_func_model fmodel;
+	u64 key = 0;
+	long addr;
 	int err;
 
 	switch (prog->type) {
@@ -2589,6 +2595,28 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 		goto out_put_prog;
 	}
 
+	if (!!tgt_prog_fd != !!btf_id) {
+		err = -EINVAL;
+		goto out_put_prog;
+	}
+
+	if (tgt_prog_fd) {
+		/* For now we only allow new targets for BPF_PROG_TYPE_EXT */
+		if (prog->type != BPF_PROG_TYPE_EXT) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
+
+		tgt_prog = bpf_prog_get(tgt_prog_fd);
+		if (IS_ERR(tgt_prog)) {
+			err = PTR_ERR(tgt_prog);
+			tgt_prog = NULL;
+			goto out_put_prog;
+		}
+
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_USER);
 	if (!link) {
 		err = -ENOMEM;
@@ -2600,12 +2628,28 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 
 	mutex_lock(&prog->aux->tgt_mutex);
 
-	if (!prog->aux->tgt_trampoline) {
+	if (!prog->aux->tgt_trampoline && !tgt_prog) {
 		err = -ENOENT;
 		goto out_unlock;
 	}
-	tr = prog->aux->tgt_trampoline;
-	tgt_prog = prog->aux->tgt_prog;
+
+	if (!prog->aux->tgt_trampoline ||
+	    (key && key != prog->aux->tgt_trampoline->key)) {
+
+		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+					      &fmodel, &addr, NULL, NULL);
+		if (err)
+			goto out_unlock;
+
+		tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
+		if (!tr) {
+			err = -ENOMEM;
+			goto out_unlock;
+		}
+	} else {
+		tr = prog->aux->tgt_trampoline;
+		tgt_prog = prog->aux->tgt_prog;
+	}
 
 	err = bpf_link_prime(&link->link, &link_primer);
 	if (err)
@@ -2620,16 +2664,24 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 
 	link->tgt_prog = tgt_prog;
 	link->trampoline = tr;
-
-	prog->aux->tgt_prog = NULL;
-	prog->aux->tgt_trampoline = NULL;
+	if (tr == prog->aux->tgt_trampoline) {
+		/* if we got a new ref from syscall, drop existing one from prog */
+		if (tgt_prog_fd)
+			bpf_prog_put(prog->aux->tgt_prog);
+		prog->aux->tgt_trampoline = NULL;
+		prog->aux->tgt_prog = NULL;
+	}
 	mutex_unlock(&prog->aux->tgt_mutex);
 
 	return bpf_link_settle(&link_primer);
 out_unlock:
+	if (tr && tr != prog->aux->tgt_trampoline)
+		bpf_trampoline_put(tr);
 	mutex_unlock(&prog->aux->tgt_mutex);
 	kfree(link);
 out_put_prog:
+	if (tgt_prog_fd && tgt_prog)
+		bpf_prog_put(tgt_prog);
 	bpf_prog_put(prog);
 	return err;
 }
@@ -2743,7 +2795,7 @@  static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 			tp_name = prog->aux->attach_func_name;
 			break;
 		}
-		return bpf_tracing_prog_attach(prog);
+		return bpf_tracing_prog_attach(prog, 0, 0);
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 		if (strncpy_from_user(buf,
@@ -3934,6 +3986,16 @@  static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
 	return -EINVAL;
 }
 
+static int freplace_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	if (attr->link_create.attach_type == prog->expected_attach_type)
+		return bpf_tracing_prog_attach(prog,
+					       attr->link_create.target_fd,
+					       attr->link_create.target_btf_id);
+	return -EINVAL;
+
+}
+
 #define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
 static int link_create(union bpf_attr *attr)
 {
@@ -3944,18 +4006,25 @@  static int link_create(union bpf_attr *attr)
 	if (CHECK_ATTR(BPF_LINK_CREATE))
 		return -EINVAL;
 
-	ptype = attach_type_to_prog_type(attr->link_create.attach_type);
-	if (ptype == BPF_PROG_TYPE_UNSPEC)
-		return -EINVAL;
-
-	prog = bpf_prog_get_type(attr->link_create.prog_fd, ptype);
+	prog = bpf_prog_get(attr->link_create.prog_fd);
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
 	ret = bpf_prog_attach_check_attach_type(prog,
 						attr->link_create.attach_type);
 	if (ret)
-		goto err_out;
+		goto out;
+
+	if (prog->type == BPF_PROG_TYPE_EXT) {
+		ret = freplace_bpf_link_attach(attr, prog);
+		goto out;
+	}
+
+	ptype = attach_type_to_prog_type(attr->link_create.attach_type);
+	if (ptype == BPF_PROG_TYPE_UNSPEC) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	switch (ptype) {
 	case BPF_PROG_TYPE_CGROUP_SKB:
@@ -3983,7 +4052,7 @@  static int link_create(union bpf_attr *attr)
 		ret = -EINVAL;
 	}
 
-err_out:
+out:
 	if (ret < 0)
 		bpf_prog_put(prog);
 	return ret;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 116f3f275b98..eeb5a4f58516 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11202,6 +11202,12 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
 
+		if ((prog->aux->tgt_prog_type &&
+		     prog->aux->tgt_prog_type != tgt_prog->type) ||
+		    (prog->aux->tgt_attach_type &&
+		     prog->aux->tgt_attach_type != tgt_prog->expected_attach_type))
+			return -EINVAL;
+
 		if (tgt_prog && conservative)
 			t = NULL;
 
@@ -11300,6 +11306,9 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 		return ret;
 
 	if (tgt_prog) {
+		prog->aux->tgt_prog_type = tgt_prog->type;
+		prog->aux->tgt_attach_type = tgt_prog->expected_attach_type;
+
 		if (prog->type == BPF_PROG_TYPE_EXT) {
 			env->ops = bpf_verifier_ops[tgt_prog->type];
 			prog->expected_attach_type =
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a22812561064..feff1ed49f86 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -632,8 +632,13 @@  union bpf_attr {
 		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
-		__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
-		__u32		iter_info_len;	/* iter_info length */
+		union {
+			__u32		target_btf_id;	/* btf_id of target to attach to */
+			struct {
+				__aligned_u64	iter_info;	/* extra bpf_iter_link_info */
+				__u32		iter_info_len;	/* iter_info length */
+			};
+		};
 	} link_create;
 
 	struct { /* struct used by BPF_LINK_UPDATE command */