On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
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;
[...]
