| Message ID | 160017005916.98230.1736872862729846213.stgit@toke.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Support multi-attach for freplace programs | expand |
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; [...]
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
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
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 >
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 --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,