diff mbox series

[bpf-next,v5,2/8] bpf: verifier: refactor check_attach_btf_id()

Message ID 160017005916.98230.1736872862729846213.stgit@toke.dk
State Superseded
Headers show
Series None | expand

Commit Message

Toke Høiland-Jørgensen Sept. 15, 2020, 11:40 a.m. UTC
From: Toke Høiland-Jørgensen <toke@redhat.com>

The check_attach_btf_id() function really does three things:

1. It performs a bunch of checks on the program to ensure that the
   attachment is valid.

2. It stores a bunch of state about the attachment being requested in
   the verifier environment and struct bpf_prog objects.

3. It allocates a trampoline for the attachment.

This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.

No functional change is intended with this patch.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/bpf.h          |    7 +
 include/linux/bpf_verifier.h |    9 ++
 kernel/bpf/trampoline.c      |   20 ++++
 kernel/bpf/verifier.c        |  197 ++++++++++++++++++++++++------------------
 4 files changed, 149 insertions(+), 84 deletions(-)

Comments

Andrii Nakryiko Sept. 16, 2020, 5:32 p.m. UTC | #1
On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The check_attach_btf_id() function really does three things:
>
> 1. It performs a bunch of checks on the program to ensure that the
>    attachment is valid.
>
> 2. It stores a bunch of state about the attachment being requested in
>    the verifier environment and struct bpf_prog objects.
>
> 3. It allocates a trampoline for the attachment.
>
> This patch splits out (1.) and (3.) into separate functions in preparation
> for reusing them when the actual attachment is happening (in the
> raw_tracepoint_open syscall operation), which will allow tracing programs
> to have multiple (compatible) attachments.
>
> No functional change is intended with this patch.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

I almost acked this, but found a problem at the very last moment. See
below, along with few more comments while I have enough context in my
head.

BTW, for whatever reason your patches arrived with a 12 hour delay
yesterday (cover letter received at 5am, while patches arrived at
6pm), don't know if its vger or gmail...

>  include/linux/bpf.h          |    7 +
>  include/linux/bpf_verifier.h |    9 ++
>  kernel/bpf/trampoline.c      |   20 ++++
>  kernel/bpf/verifier.c        |  197 ++++++++++++++++++++++++------------------
>  4 files changed, 149 insertions(+), 84 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5ad4a935a24e..dcf0c70348a4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -616,6 +616,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
>  struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
>  int bpf_trampoline_link_prog(struct bpf_prog *prog);
>  int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
> +struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                         struct btf_func_model *fmodel);
>  void bpf_trampoline_put(struct bpf_trampoline *tr);
>  #define BPF_DISPATCHER_INIT(_name) {                           \
>         .mutex = __MUTEX_INITIALIZER(_name.mutex),              \
> @@ -672,6 +674,11 @@ static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
>  {
>         return -ENOTSUPP;
>  }
> +static inline struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                                       struct btf_func_model *fmodel)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
>  static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
>  #define DEFINE_BPF_DISPATCHER(name)
>  #define DECLARE_BPF_DISPATCHER(name)
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 20009e766805..db3db0b69aad 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -447,4 +447,13 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>  int check_ctx_reg(struct bpf_verifier_env *env,
>                   const struct bpf_reg_state *reg, int regno);
>
> +int bpf_check_attach_target(struct bpf_verifier_log *log,
> +                           const struct bpf_prog *prog,
> +                           const struct bpf_prog *tgt_prog,
> +                           u32 btf_id,
> +                           struct btf_func_model *fmodel,
> +                           long *tgt_addr,
> +                           const char **tgt_name,
> +                           const struct btf_type **tgt_type);

So this is obviously an abomination of a function signature,
especially for a one exported to other files.

One candidate to remove would be tgt_type, which is supposed to be a
derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
**except** (and that's how I found the bug below), in case of
fentry/fexit programs attaching to "conservative" BPF functions, in
which case what's stored in aux->attach_func_proto is different from
what is passed into btf_distill_func_proto. So that's a bug already
(you'll return NULL in some cases for tgt_type, while it has to always
be non-NULL).

But related to that is fmodel. It seems like bpf_check_attach_target()
has no interest in fmodel itself and is just passing it from
btf_distill_func_proto(). So I was about to suggest dropping fmodel
and calling btf_distill_func_proto() outside of
bpf_check_attach_target(), but given the conservative + fentry/fexit
quirk, it's probably going to be more confusing.

So with all this, I suggest dropping the tgt_type output param
altogether and let callers do a `btf__type_by_id(tgt_prog ?
tgt_prog->aux->btf : btf_vmlinux, btf_id);`. That will both fix the
bug and will make this function's signature just a tad bit less
horrible.

> +
>  #endif /* _LINUX_BPF_VERIFIER_H */
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 7dd523a7e32d..7845913e7e41 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -336,6 +336,26 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
>         return err;
>  }
>
> +struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
> +                                         struct btf_func_model *fmodel)
> +{
> +       struct bpf_trampoline *tr;
> +
> +       tr = bpf_trampoline_lookup(key);
> +       if (!tr)
> +               return ERR_PTR(-ENOMEM);

So seems like the only way this function can fail is when
bpf_trampoline_lookup() returns NULL (and we assume -ENOMEM then), so
I guess we could have just returned NULL the same to keep
bpf_trampoline_lookup() and bpf_trampoline_get() similar. But it's
minor, if you prefer to encode error code anyways.

> +
> +       mutex_lock(&tr->mutex);
> +       if (tr->func.addr)
> +               goto out;
> +
> +       memcpy(&tr->func.model, fmodel, sizeof(*fmodel));
> +       tr->func.addr = addr;
> +out:
> +       mutex_unlock(&tr->mutex);
> +       return tr;
> +}
> +
>  void bpf_trampoline_put(struct bpf_trampoline *tr)
>  {
>         if (!tr)

[...]

> @@ -11235,24 +11202,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>                 t = btf_type_by_id(btf, t->type);
>                 if (!btf_type_is_func_proto(t))
>                         return -EINVAL;
> -               tr = bpf_trampoline_lookup(key);
> -               if (!tr)
> -                       return -ENOMEM;
> -               /* t is either vmlinux type or another program's type */
> -               prog->aux->attach_func_proto = t;
> -               mutex_lock(&tr->mutex);
> -               if (tr->func.addr) {
> -                       prog->aux->trampoline = tr;
> -                       goto out;
> -               }
> -               if (tgt_prog && conservative) {
> -                       prog->aux->attach_func_proto = NULL;
> +
> +               if (tgt_prog && conservative)
>                         t = NULL;

this is where the bug happens, we can't return this NULL to caller as tgt_prog

> -               }
> -               ret = btf_distill_func_proto(log, btf, t,
> -                                            tname, &tr->func.model);
> +
> +               ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
>                 if (ret < 0)
> -                       goto out;
> +                       return ret;
> +
>                 if (tgt_prog) {
>                         if (subprog == 0)
>                                 addr = (long) tgt_prog->bpf_func;

[...]
Toke Høiland-Jørgensen Sept. 16, 2020, 9:07 p.m. UTC | #2
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> The check_attach_btf_id() function really does three things:
>>
>> 1. It performs a bunch of checks on the program to ensure that the
>>    attachment is valid.
>>
>> 2. It stores a bunch of state about the attachment being requested in
>>    the verifier environment and struct bpf_prog objects.
>>
>> 3. It allocates a trampoline for the attachment.
>>
>> This patch splits out (1.) and (3.) into separate functions in preparation
>> for reusing them when the actual attachment is happening (in the
>> raw_tracepoint_open syscall operation), which will allow tracing programs
>> to have multiple (compatible) attachments.
>>
>> No functional change is intended with this patch.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>
> I almost acked this, but found a problem at the very last moment. See
> below, along with few more comments while I have enough context in my
> head.

Right, will fix, thanks!

> BTW, for whatever reason your patches arrived with a 12 hour delay
> yesterday (cover letter received at 5am, while patches arrived at
> 6pm), don't know if its vger or gmail...

Ugh, sorry about that. I think it's an interaction between vger and the
Red Hat corporate mail proxy - it's really a mess. I'll try switching my
patch submissions to use a different SMTP server...

-Toke
Toke Høiland-Jørgensen Sept. 17, 2020, 10:06 a.m. UTC | #3
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

>>

>> +int bpf_check_attach_target(struct bpf_verifier_log *log,

>> +                           const struct bpf_prog *prog,

>> +                           const struct bpf_prog *tgt_prog,

>> +                           u32 btf_id,

>> +                           struct btf_func_model *fmodel,

>> +                           long *tgt_addr,

>> +                           const char **tgt_name,

>> +                           const struct btf_type **tgt_type);

>

> So this is obviously an abomination of a function signature,

> especially for a one exported to other files.

>

> One candidate to remove would be tgt_type, which is supposed to be a

> derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,

> **except** (and that's how I found the bug below), in case of

> fentry/fexit programs attaching to "conservative" BPF functions, in

> which case what's stored in aux->attach_func_proto is different from

> what is passed into btf_distill_func_proto. So that's a bug already

> (you'll return NULL in some cases for tgt_type, while it has to always

> be non-NULL).


Okay, looked at this in more detail, and I don't think the refactored
code is doing anything different from the pre-refactor version?

Before we had this:

		if (tgt_prog && conservative) {
			prog->aux->attach_func_proto = NULL;
			t = NULL;
		}

and now we just have

		if (tgt_prog && conservative)
			t = NULL;

in bpf_check_attach_target(), which gets returned as tgt_type and
subsequently assigned to prog->aux->attach_func_proto.

> But related to that is fmodel. It seems like bpf_check_attach_target()

> has no interest in fmodel itself and is just passing it from

> btf_distill_func_proto(). So I was about to suggest dropping fmodel

> and calling btf_distill_func_proto() outside of

> bpf_check_attach_target(), but given the conservative + fentry/fexit

> quirk, it's probably going to be more confusing.

>

> So with all this, I suggest dropping the tgt_type output param

> altogether and let callers do a `btf__type_by_id(tgt_prog ?

> tgt_prog->aux->btf : btf_vmlinux, btf_id);`. That will both fix the

> bug and will make this function's signature just a tad bit less

> horrible.


Thought about this, but the logic also does a few transformations of the
type itself, e.g., this for bpf_trace_raw_tp:

		tname += sizeof(prefix) - 1;
		t = btf_type_by_id(btf, t->type);
		if (!btf_type_is_ptr(t))
			/* should never happen in valid vmlinux build */
			return -EINVAL;
		t = btf_type_by_id(btf, t->type);
		if (!btf_type_is_func_proto(t))
			/* should never happen in valid vmlinux build */
			return -EINVAL;

so to catch this we really do have to return the type from the function
as well.

I do agree that the function signature is a tad on the long side, but I
couldn't think of any good way of making it smaller. I considered
replacing the last two return values with a boolean 'save' parameter,
that would just make it same the values directly in prog->aux; but I
actually find it easier to reason about a function that is strictly
checking things and returning the result, instead of 'sometimes modify'
semantics...

-Toke
Andrii Nakryiko Sept. 17, 2020, 4:38 p.m. UTC | #4
On Thu, Sep 17, 2020 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> >>
> >> +int bpf_check_attach_target(struct bpf_verifier_log *log,
> >> +                           const struct bpf_prog *prog,
> >> +                           const struct bpf_prog *tgt_prog,
> >> +                           u32 btf_id,
> >> +                           struct btf_func_model *fmodel,
> >> +                           long *tgt_addr,
> >> +                           const char **tgt_name,
> >> +                           const struct btf_type **tgt_type);
> >
> > So this is obviously an abomination of a function signature,
> > especially for a one exported to other files.
> >
> > One candidate to remove would be tgt_type, which is supposed to be a
> > derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
> > **except** (and that's how I found the bug below), in case of
> > fentry/fexit programs attaching to "conservative" BPF functions, in
> > which case what's stored in aux->attach_func_proto is different from
> > what is passed into btf_distill_func_proto. So that's a bug already
> > (you'll return NULL in some cases for tgt_type, while it has to always
> > be non-NULL).
>
> Okay, looked at this in more detail, and I don't think the refactored
> code is doing anything different from the pre-refactor version?
>
> Before we had this:
>
>                 if (tgt_prog && conservative) {
>                         prog->aux->attach_func_proto = NULL;
>                         t = NULL;
>                 }
>
> and now we just have
>
>                 if (tgt_prog && conservative)
>                         t = NULL;
>
> in bpf_check_attach_target(), which gets returned as tgt_type and
> subsequently assigned to prog->aux->attach_func_proto.

Yeah, you are totally right, I don't know how I missed that
`prog->aux->attach_func_proto = NULL;`, sorry about that.

>
> > But related to that is fmodel. It seems like bpf_check_attach_target()
> > has no interest in fmodel itself and is just passing it from
> > btf_distill_func_proto(). So I was about to suggest dropping fmodel
> > and calling btf_distill_func_proto() outside of
> > bpf_check_attach_target(), but given the conservative + fentry/fexit
> > quirk, it's probably going to be more confusing.
> >
> > So with all this, I suggest dropping the tgt_type output param
> > altogether and let callers do a `btf__type_by_id(tgt_prog ?
> > tgt_prog->aux->btf : btf_vmlinux, btf_id);`. That will both fix the
> > bug and will make this function's signature just a tad bit less
> > horrible.
>
> Thought about this, but the logic also does a few transformations of the
> type itself, e.g., this for bpf_trace_raw_tp:
>
>                 tname += sizeof(prefix) - 1;
>                 t = btf_type_by_id(btf, t->type);
>                 if (!btf_type_is_ptr(t))
>                         /* should never happen in valid vmlinux build */
>                         return -EINVAL;
>                 t = btf_type_by_id(btf, t->type);
>                 if (!btf_type_is_func_proto(t))
>                         /* should never happen in valid vmlinux build */
>                         return -EINVAL;
>
> so to catch this we really do have to return the type from the function
> as well.

yeah, with func_proto sometimes being null, btf_id isn't enough, so
that can't be done anyways.

>
> I do agree that the function signature is a tad on the long side, but I
> couldn't think of any good way of making it smaller. I considered
> replacing the last two return values with a boolean 'save' parameter,
> that would just make it same the values directly in prog->aux; but I
> actually find it easier to reason about a function that is strictly
> checking things and returning the result, instead of 'sometimes modify'
> semantics...

I agree, modifying prog->aux would be worse. And
btf_distill_func_proto() can't be extracted right away, because it
doesn't happen for the RAW_TP case. Oh well, we'll have to live with
an 8-argument function, I suppose.

Please add my ack when you post a new version:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> -Toke
>
Toke Høiland-Jørgensen Sept. 17, 2020, 4:54 p.m. UTC | #5
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 17, 2020 at 3:06 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> >>
>> >> +int bpf_check_attach_target(struct bpf_verifier_log *log,
>> >> +                           const struct bpf_prog *prog,
>> >> +                           const struct bpf_prog *tgt_prog,
>> >> +                           u32 btf_id,
>> >> +                           struct btf_func_model *fmodel,
>> >> +                           long *tgt_addr,
>> >> +                           const char **tgt_name,
>> >> +                           const struct btf_type **tgt_type);
>> >
>> > So this is obviously an abomination of a function signature,
>> > especially for a one exported to other files.
>> >
>> > One candidate to remove would be tgt_type, which is supposed to be a
>> > derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
>> > **except** (and that's how I found the bug below), in case of
>> > fentry/fexit programs attaching to "conservative" BPF functions, in
>> > which case what's stored in aux->attach_func_proto is different from
>> > what is passed into btf_distill_func_proto. So that's a bug already
>> > (you'll return NULL in some cases for tgt_type, while it has to always
>> > be non-NULL).
>>
>> Okay, looked at this in more detail, and I don't think the refactored
>> code is doing anything different from the pre-refactor version?
>>
>> Before we had this:
>>
>>                 if (tgt_prog && conservative) {
>>                         prog->aux->attach_func_proto = NULL;
>>                         t = NULL;
>>                 }
>>
>> and now we just have
>>
>>                 if (tgt_prog && conservative)
>>                         t = NULL;
>>
>> in bpf_check_attach_target(), which gets returned as tgt_type and
>> subsequently assigned to prog->aux->attach_func_proto.
>
> Yeah, you are totally right, I don't know how I missed that
> `prog->aux->attach_func_proto = NULL;`, sorry about that.

No worries - this was certainly not the easiest to review; thanks for
sticking with it! :)

[..]

> Please add my ack when you post a new version:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>

Will do, thanks!

-Toke
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5ad4a935a24e..dcf0c70348a4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -616,6 +616,8 @@  static __always_inline unsigned int bpf_dispatcher_nop_func(
 struct bpf_trampoline *bpf_trampoline_lookup(u64 key);
 int bpf_trampoline_link_prog(struct bpf_prog *prog);
 int bpf_trampoline_unlink_prog(struct bpf_prog *prog);
+struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+					  struct btf_func_model *fmodel);
 void bpf_trampoline_put(struct bpf_trampoline *tr);
 #define BPF_DISPATCHER_INIT(_name) {				\
 	.mutex = __MUTEX_INITIALIZER(_name.mutex),		\
@@ -672,6 +674,11 @@  static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 {
 	return -ENOTSUPP;
 }
+static inline struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+							struct btf_func_model *fmodel)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
 static inline void bpf_trampoline_put(struct bpf_trampoline *tr) {}
 #define DEFINE_BPF_DISPATCHER(name)
 #define DECLARE_BPF_DISPATCHER(name)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 20009e766805..db3db0b69aad 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -447,4 +447,13 @@  bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
 int check_ctx_reg(struct bpf_verifier_env *env,
 		  const struct bpf_reg_state *reg, int regno);
 
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type);
+
 #endif /* _LINUX_BPF_VERIFIER_H */
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7dd523a7e32d..7845913e7e41 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -336,6 +336,26 @@  int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	return err;
 }
 
+struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
+					  struct btf_func_model *fmodel)
+{
+	struct bpf_trampoline *tr;
+
+	tr = bpf_trampoline_lookup(key);
+	if (!tr)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&tr->mutex);
+	if (tr->func.addr)
+		goto out;
+
+	memcpy(&tr->func.model, fmodel, sizeof(*fmodel));
+	tr->func.addr = addr;
+out:
+	mutex_unlock(&tr->mutex);
+	return tr;
+}
+
 void bpf_trampoline_put(struct bpf_trampoline *tr)
 {
 	if (!tr)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0be7a187fb7f..d38678319ca4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10997,11 +10997,11 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 }
 #define SECURITY_PREFIX "security_"
 
-static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
+static int check_attach_modify_return(const struct bpf_prog *prog, unsigned long addr,
+				      const char *func_name)
 {
 	if (within_error_injection_list(addr) ||
-	    !strncmp(SECURITY_PREFIX, prog->aux->attach_func_name,
-		     sizeof(SECURITY_PREFIX) - 1))
+	    !strncmp(SECURITY_PREFIX, func_name, sizeof(SECURITY_PREFIX) - 1))
 		return 0;
 
 	return -EINVAL;
@@ -11038,43 +11038,29 @@  static int check_non_sleepable_error_inject(u32 btf_id)
 	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }
 
-static int check_attach_btf_id(struct bpf_verifier_env *env)
+int bpf_check_attach_target(struct bpf_verifier_log *log,
+			    const struct bpf_prog *prog,
+			    const struct bpf_prog *tgt_prog,
+			    u32 btf_id,
+			    struct btf_func_model *fmodel,
+			    long *tgt_addr,
+			    const char **tgt_name,
+			    const struct btf_type **tgt_type)
 {
-	struct bpf_prog *prog = env->prog;
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
-	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
-	struct bpf_verifier_log *log = &env->log;
-	u32 btf_id = prog->aux->attach_btf_id;
 	const char prefix[] = "btf_trace_";
-	struct btf_func_model fmodel;
 	int ret = 0, subprog = -1, i;
-	struct bpf_trampoline *tr;
 	const struct btf_type *t;
 	bool conservative = true;
 	const char *tname;
 	struct btf *btf;
-	long addr;
-	u64 key;
-
-	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM) {
-		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
-		return -EINVAL;
-	}
-
-	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
-		return check_struct_ops_btf_id(env);
-
-	if (prog->type != BPF_PROG_TYPE_TRACING &&
-	    prog->type != BPF_PROG_TYPE_LSM &&
-	    !prog_extension)
-		return 0;
+	long addr = 0;
 
 	if (!btf_id) {
 		bpf_log(log, "Tracing programs must provide btf_id\n");
 		return -EINVAL;
 	}
-	btf = bpf_prog_get_target_btf(prog);
+	btf = tgt_prog ? tgt_prog->aux->btf : btf_vmlinux;
 	if (!btf) {
 		bpf_log(log,
 			"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
@@ -11114,8 +11100,6 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 					"Extension programs should be JITed\n");
 				return -EINVAL;
 			}
-			env->ops = bpf_verifier_ops[tgt_prog->type];
-			prog->expected_attach_type = tgt_prog->expected_attach_type;
 		}
 		if (!tgt_prog->jited) {
 			bpf_log(log, "Can attach to only JITed progs\n");
@@ -11151,13 +11135,11 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 			bpf_log(log, "Cannot extend fentry/fexit\n");
 			return -EINVAL;
 		}
-		key = ((u64)aux->id) << 32 | btf_id;
 	} else {
 		if (prog_extension) {
 			bpf_log(log, "Cannot replace kernel functions\n");
 			return -EINVAL;
 		}
-		key = btf_id;
 	}
 
 	switch (prog->expected_attach_type) {
@@ -11187,13 +11169,7 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 			/* should never happen in valid vmlinux build */
 			return -EINVAL;
 
-		/* remember two read only pointers that are valid for
-		 * the life time of the kernel
-		 */
-		prog->aux->attach_func_name = tname;
-		prog->aux->attach_func_proto = t;
-		prog->aux->attach_btf_trace = true;
-		return 0;
+		break;
 	case BPF_TRACE_ITER:
 		if (!btf_type_is_func(t)) {
 			bpf_log(log, "attach_btf_id %u is not a function\n",
@@ -11203,12 +11179,10 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 		t = btf_type_by_id(btf, t->type);
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
-		prog->aux->attach_func_name = tname;
-		prog->aux->attach_func_proto = t;
-		if (!bpf_iter_prog_supported(prog))
-			return -EINVAL;
-		ret = btf_distill_func_proto(log, btf, t, tname, &fmodel);
-		return ret;
+		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
+		if (ret)
+			return ret;
+		break;
 	default:
 		if (!prog_extension)
 			return -EINVAL;
@@ -11217,13 +11191,6 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 	case BPF_LSM_MAC:
 	case BPF_TRACE_FENTRY:
 	case BPF_TRACE_FEXIT:
-		prog->aux->attach_func_name = tname;
-		if (prog->type == BPF_PROG_TYPE_LSM) {
-			ret = bpf_lsm_verify_prog(log, prog);
-			if (ret < 0)
-				return ret;
-		}
-
 		if (!btf_type_is_func(t)) {
 			bpf_log(log, "attach_btf_id %u is not a function\n",
 				btf_id);
@@ -11235,24 +11202,14 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 		t = btf_type_by_id(btf, t->type);
 		if (!btf_type_is_func_proto(t))
 			return -EINVAL;
-		tr = bpf_trampoline_lookup(key);
-		if (!tr)
-			return -ENOMEM;
-		/* t is either vmlinux type or another program's type */
-		prog->aux->attach_func_proto = t;
-		mutex_lock(&tr->mutex);
-		if (tr->func.addr) {
-			prog->aux->trampoline = tr;
-			goto out;
-		}
-		if (tgt_prog && conservative) {
-			prog->aux->attach_func_proto = NULL;
+
+		if (tgt_prog && conservative)
 			t = NULL;
-		}
-		ret = btf_distill_func_proto(log, btf, t,
-					     tname, &tr->func.model);
+
+		ret = btf_distill_func_proto(log, btf, t, tname, fmodel);
 		if (ret < 0)
-			goto out;
+			return ret;
+
 		if (tgt_prog) {
 			if (subprog == 0)
 				addr = (long) tgt_prog->bpf_func;
@@ -11264,8 +11221,7 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 				bpf_log(log,
 					"The address of function %s cannot be found\n",
 					tname);
-				ret = -ENOENT;
-				goto out;
+				return -ENOENT;
 			}
 		}
 
@@ -11290,25 +11246,98 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 			default:
 				break;
 			}
-			if (ret)
-				bpf_log(log, "%s is not sleepable\n",
-					prog->aux->attach_func_name);
+			if (ret) {
+				bpf_log(log, "%s is not sleepable\n", tname);
+				return ret;
+			}
 		} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
-			ret = check_attach_modify_return(prog, addr);
-			if (ret)
-				bpf_log(log, "%s() is not modifiable\n",
-					prog->aux->attach_func_name);
+			ret = check_attach_modify_return(prog, addr, tname);
+			if (ret) {
+				bpf_log(log, "%s() is not modifiable\n", tname);
+				return ret;
+			}
 		}
-		if (ret)
-			goto out;
-		tr->func.addr = (void *)addr;
-		prog->aux->trampoline = tr;
-out:
-		mutex_unlock(&tr->mutex);
-		if (ret)
-			bpf_trampoline_put(tr);
+
+		break;
+	}
+	*tgt_addr = addr;
+	if (tgt_name)
+		*tgt_name = tname;
+	if (tgt_type)
+		*tgt_type = t;
+	return 0;
+}
+
+static int check_attach_btf_id(struct bpf_verifier_env *env)
+{
+	struct bpf_prog *prog = env->prog;
+	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+	u32 btf_id = prog->aux->attach_btf_id;
+	struct btf_func_model fmodel;
+	struct bpf_trampoline *tr;
+	const struct btf_type *t;
+	const char *tname;
+	long addr;
+	int ret;
+	u64 key;
+
+	if (prog->aux->sleepable && prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM) {
+		verbose(env, "Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");
+		return -EINVAL;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
+		return check_struct_ops_btf_id(env);
+
+	if (prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM &&
+	    prog->type != BPF_PROG_TYPE_EXT)
+		return 0;
+
+	ret = bpf_check_attach_target(&env->log, prog, tgt_prog, btf_id,
+				      &fmodel, &addr, &tname, &t);
+	if (ret)
 		return ret;
+
+	if (tgt_prog) {
+		if (prog->type == BPF_PROG_TYPE_EXT) {
+			env->ops = bpf_verifier_ops[tgt_prog->type];
+			prog->expected_attach_type =
+				tgt_prog->expected_attach_type;
+		}
+		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+	} else {
+		key = btf_id;
 	}
+
+	/* remember two read only pointers that are valid for
+	 * the life time of the kernel
+	 */
+	prog->aux->attach_func_proto = t;
+	prog->aux->attach_func_name = tname;
+
+	if (prog->expected_attach_type == BPF_TRACE_RAW_TP) {
+		prog->aux->attach_btf_trace = true;
+		return 0;
+	} else if (prog->expected_attach_type == BPF_TRACE_ITER) {
+		if (!bpf_iter_prog_supported(prog))
+			return -EINVAL;
+		return 0;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_LSM) {
+		ret = bpf_lsm_verify_prog(&env->log, prog);
+		if (ret < 0)
+			return ret;
+	}
+
+	tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
+	if (IS_ERR(tr))
+		return PTR_ERR(tr);
+
+	prog->aux->trampoline = tr;
+	return 0;
 }
 
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,