* [PATCH bpf-next v5 1/8] bpf: change logging calls from verbose() to bpf_log() and use log pointer
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
@ 2020-09-15 11:40 ` Toke Høiland-Jørgensen
2020-09-16 17:08 ` Andrii Nakryiko
2020-09-15 11:40 ` [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id() Toke Høiland-Jørgensen
` (6 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Toke Høiland-Jørgensen <toke@redhat.com>
In preparation for moving code around, change a bunch of references to
env->log (and the verbose() logging helper) to use bpf_log() and a direct
pointer to struct bpf_verifier_log. While we're touching the function
signature, mark the 'prog' argument to bpf_check_type_match() as const.
Also enhance the bpf_verifier_log_needed() check to handle NULL pointers
for the log struct so we can re-use the code with logging disabled.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/bpf.h | 2 +-
include/linux/bpf_verifier.h | 5 +++-
kernel/bpf/btf.c | 6 +++--
kernel/bpf/verifier.c | 48 +++++++++++++++++++++---------------------
4 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c6d9f2c444f4..5ad4a935a24e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1394,7 +1394,7 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
struct bpf_reg_state *regs);
int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
struct bpf_reg_state *reg);
-int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
+int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
struct btf *btf, const struct btf_type *t);
struct bpf_prog *bpf_prog_by_id(u32 id);
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 53c7bd568c5d..20009e766805 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -347,8 +347,9 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
{
- return (log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
- log->level == BPF_LOG_KERNEL;
+ return log &&
+ ((log->level && log->ubuf && !bpf_verifier_log_full(log)) ||
+ log->level == BPF_LOG_KERNEL);
}
#define BPF_MAX_SUBPROGS 256
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f9ac6935ab3c..2ace56c99c36 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4401,7 +4401,7 @@ static int btf_check_func_type_match(struct bpf_verifier_log *log,
}
/* Compare BTFs of given program with BTF of target program */
-int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
+int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *prog,
struct btf *btf2, const struct btf_type *t2)
{
struct btf *btf1 = prog->aux->btf;
@@ -4409,7 +4409,7 @@ int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
u32 btf_id = 0;
if (!prog->aux->func_info) {
- bpf_log(&env->log, "Program extension requires BTF\n");
+ bpf_log(log, "Program extension requires BTF\n");
return -EINVAL;
}
@@ -4421,7 +4421,7 @@ int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,
if (!t1 || !btf_type_is_func(t1))
return -EFAULT;
- return btf_check_func_type_match(&env->log, btf1, t1, btf2, t2);
+ return btf_check_func_type_match(log, btf1, t1, btf2, t2);
}
/* Compare BTF of a function with given bpf_reg_state.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 814bc6c1ad16..0be7a187fb7f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11043,6 +11043,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
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;
@@ -11070,23 +11071,23 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
return 0;
if (!btf_id) {
- verbose(env, "Tracing programs must provide btf_id\n");
+ bpf_log(log, "Tracing programs must provide btf_id\n");
return -EINVAL;
}
btf = bpf_prog_get_target_btf(prog);
if (!btf) {
- verbose(env,
+ bpf_log(log,
"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
return -EINVAL;
}
t = btf_type_by_id(btf, btf_id);
if (!t) {
- verbose(env, "attach_btf_id %u is invalid\n", btf_id);
+ bpf_log(log, "attach_btf_id %u is invalid\n", btf_id);
return -EINVAL;
}
tname = btf_name_by_offset(btf, t->name_off);
if (!tname) {
- verbose(env, "attach_btf_id %u doesn't have a name\n", btf_id);
+ bpf_log(log, "attach_btf_id %u doesn't have a name\n", btf_id);
return -EINVAL;
}
if (tgt_prog) {
@@ -11098,18 +11099,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
break;
}
if (subprog == -1) {
- verbose(env, "Subprog %s doesn't exist\n", tname);
+ bpf_log(log, "Subprog %s doesn't exist\n", tname);
return -EINVAL;
}
conservative = aux->func_info_aux[subprog].unreliable;
if (prog_extension) {
if (conservative) {
- verbose(env,
+ bpf_log(log,
"Cannot replace static functions\n");
return -EINVAL;
}
if (!prog->jit_requested) {
- verbose(env,
+ bpf_log(log,
"Extension programs should be JITed\n");
return -EINVAL;
}
@@ -11117,7 +11118,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
prog->expected_attach_type = tgt_prog->expected_attach_type;
}
if (!tgt_prog->jited) {
- verbose(env, "Can attach to only JITed progs\n");
+ bpf_log(log, "Can attach to only JITed progs\n");
return -EINVAL;
}
if (tgt_prog->type == prog->type) {
@@ -11125,7 +11126,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
* Cannot attach program extension to another extension.
* It's ok to attach fentry/fexit to extension program.
*/
- verbose(env, "Cannot recursively attach\n");
+ bpf_log(log, "Cannot recursively attach\n");
return -EINVAL;
}
if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
@@ -11147,13 +11148,13 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
* reasonable stack size. Hence extending fentry is not
* allowed.
*/
- verbose(env, "Cannot extend fentry/fexit\n");
+ bpf_log(log, "Cannot extend fentry/fexit\n");
return -EINVAL;
}
key = ((u64)aux->id) << 32 | btf_id;
} else {
if (prog_extension) {
- verbose(env, "Cannot replace kernel functions\n");
+ bpf_log(log, "Cannot replace kernel functions\n");
return -EINVAL;
}
key = btf_id;
@@ -11162,17 +11163,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
switch (prog->expected_attach_type) {
case BPF_TRACE_RAW_TP:
if (tgt_prog) {
- verbose(env,
+ bpf_log(log,
"Only FENTRY/FEXIT progs are attachable to another BPF prog\n");
return -EINVAL;
}
if (!btf_type_is_typedef(t)) {
- verbose(env, "attach_btf_id %u is not a typedef\n",
+ bpf_log(log, "attach_btf_id %u is not a typedef\n",
btf_id);
return -EINVAL;
}
if (strncmp(prefix, tname, sizeof(prefix) - 1)) {
- verbose(env, "attach_btf_id %u points to wrong type name %s\n",
+ bpf_log(log, "attach_btf_id %u points to wrong type name %s\n",
btf_id, tname);
return -EINVAL;
}
@@ -11195,7 +11196,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
return 0;
case BPF_TRACE_ITER:
if (!btf_type_is_func(t)) {
- verbose(env, "attach_btf_id %u is not a function\n",
+ bpf_log(log, "attach_btf_id %u is not a function\n",
btf_id);
return -EINVAL;
}
@@ -11206,8 +11207,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
prog->aux->attach_func_proto = t;
if (!bpf_iter_prog_supported(prog))
return -EINVAL;
- ret = btf_distill_func_proto(&env->log, btf, t,
- tname, &fmodel);
+ ret = btf_distill_func_proto(log, btf, t, tname, &fmodel);
return ret;
default:
if (!prog_extension)
@@ -11219,18 +11219,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
case BPF_TRACE_FEXIT:
prog->aux->attach_func_name = tname;
if (prog->type == BPF_PROG_TYPE_LSM) {
- ret = bpf_lsm_verify_prog(&env->log, prog);
+ ret = bpf_lsm_verify_prog(log, prog);
if (ret < 0)
return ret;
}
if (!btf_type_is_func(t)) {
- verbose(env, "attach_btf_id %u is not a function\n",
+ bpf_log(log, "attach_btf_id %u is not a function\n",
btf_id);
return -EINVAL;
}
if (prog_extension &&
- btf_check_type_match(env, prog, btf, t))
+ btf_check_type_match(log, prog, btf, t))
return -EINVAL;
t = btf_type_by_id(btf, t->type);
if (!btf_type_is_func_proto(t))
@@ -11249,7 +11249,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
prog->aux->attach_func_proto = NULL;
t = NULL;
}
- ret = btf_distill_func_proto(&env->log, btf, t,
+ ret = btf_distill_func_proto(log, btf, t,
tname, &tr->func.model);
if (ret < 0)
goto out;
@@ -11261,7 +11261,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
} else {
addr = kallsyms_lookup_name(tname);
if (!addr) {
- verbose(env,
+ bpf_log(log,
"The address of function %s cannot be found\n",
tname);
ret = -ENOENT;
@@ -11291,12 +11291,12 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
break;
}
if (ret)
- verbose(env, "%s is not sleepable\n",
+ bpf_log(log, "%s is not sleepable\n",
prog->aux->attach_func_name);
} else if (prog->expected_attach_type == BPF_MODIFY_RETURN) {
ret = check_attach_modify_return(prog, addr);
if (ret)
- verbose(env, "%s() is not modifiable\n",
+ bpf_log(log, "%s() is not modifiable\n",
prog->aux->attach_func_name);
}
if (ret)
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 1/8] bpf: change logging calls from verbose() to bpf_log() and use log pointer
2020-09-15 11:40 ` [PATCH bpf-next v5 1/8] bpf: change logging calls from verbose() to bpf_log() and use log pointer Toke Høiland-Jørgensen
@ 2020-09-16 17:08 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 17:08 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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>
>
> In preparation for moving code around, change a bunch of references to
> env->log (and the verbose() logging helper) to use bpf_log() and a direct
> pointer to struct bpf_verifier_log. While we're touching the function
> signature, mark the 'prog' argument to bpf_check_type_match() as const.
>
> Also enhance the bpf_verifier_log_needed() check to handle NULL pointers
> for the log struct so we can re-use the code with logging disabled.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
Ok, let's get this out of the way :)
Acked-by: Andrii Nakryiko <andriin@fb.com>
> include/linux/bpf.h | 2 +-
> include/linux/bpf_verifier.h | 5 +++-
> kernel/bpf/btf.c | 6 +++--
> kernel/bpf/verifier.c | 48 +++++++++++++++++++++---------------------
> 4 files changed, 31 insertions(+), 30 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
2020-09-15 11:40 ` [PATCH bpf-next v5 1/8] bpf: change logging calls from verbose() to bpf_log() and use log pointer Toke Høiland-Jørgensen
@ 2020-09-15 11:40 ` Toke Høiland-Jørgensen
2020-09-16 17:32 ` Andrii Nakryiko
2020-09-15 11:41 ` [PATCH bpf-next v5 3/8] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach Toke Høiland-Jørgensen
` (5 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
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(-)
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,
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-15 11:40 ` [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id() Toke Høiland-Jørgensen
@ 2020-09-16 17:32 ` Andrii Nakryiko
2020-09-16 21:07 ` Toke Høiland-Jørgensen
2020-09-17 10:06 ` Toke Høiland-Jørgensen
0 siblings, 2 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 17:32 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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;
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-16 17:32 ` Andrii Nakryiko
@ 2020-09-16 21:07 ` Toke Høiland-Jørgensen
2020-09-17 10:06 ` Toke Høiland-Jørgensen
1 sibling, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:07 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-16 17:32 ` Andrii Nakryiko
2020-09-16 21:07 ` Toke Høiland-Jørgensen
@ 2020-09-17 10:06 ` Toke Høiland-Jørgensen
2020-09-17 16:38 ` Andrii Nakryiko
1 sibling, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-17 10:06 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-17 10:06 ` Toke Høiland-Jørgensen
@ 2020-09-17 16:38 ` Andrii Nakryiko
2020-09-17 16:54 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-17 16:38 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id()
2020-09-17 16:38 ` Andrii Nakryiko
@ 2020-09-17 16:54 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-17 16:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 3/8] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
2020-09-15 11:40 ` [PATCH bpf-next v5 1/8] bpf: change logging calls from verbose() to bpf_log() and use log pointer Toke Høiland-Jørgensen
2020-09-15 11:40 ` [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf_id() Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-16 18:22 ` Andrii Nakryiko
2020-09-15 11:41 ` [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points Toke Høiland-Jørgensen
` (4 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Toke Høiland-Jørgensen <toke@redhat.com>
In preparation for allowing multiple attachments of freplace programs, move
the references to the target program and trampoline into the
bpf_tracing_link structure when that is created. To do this atomically,
introduce a new mutex in prog->aux to protect writing to the two pointers
to target prog and trampoline, and rename the members to make it clear that
they are related.
With this change, it is no longer possible to attach the same tracing
program multiple times (detaching in-between), since the reference from the
tracing program to the target disappears on the first attach. However,
since the next patch will let the caller supply an attach target, that will
also make it possible to attach to the same place multiple times.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/bpf.h | 15 +++++++++------
kernel/bpf/btf.c | 6 +++---
kernel/bpf/core.c | 9 ++++++---
kernel/bpf/syscall.c | 46 ++++++++++++++++++++++++++++++++++++++++------
kernel/bpf/trampoline.c | 12 ++++--------
kernel/bpf/verifier.c | 9 +++++----
6 files changed, 67 insertions(+), 30 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index dcf0c70348a4..939b37c78d55 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -614,8 +614,8 @@ static __always_inline unsigned int bpf_dispatcher_nop_func(
}
#ifdef CONFIG_BPF_JIT
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);
+int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr);
+int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr);
struct bpf_trampoline *bpf_trampoline_get(u64 key, void *addr,
struct btf_func_model *fmodel);
void bpf_trampoline_put(struct bpf_trampoline *tr);
@@ -666,11 +666,13 @@ static inline struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
{
return NULL;
}
-static inline int bpf_trampoline_link_prog(struct bpf_prog *prog)
+static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
+ struct bpf_trampoline *tr)
{
return -ENOTSUPP;
}
-static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
+static inline int bpf_trampoline_unlink_prog(struct bpf_prog *prog,
+ struct bpf_trampoline *tr)
{
return -ENOTSUPP;
}
@@ -738,14 +740,15 @@ struct bpf_prog_aux {
u32 max_rdonly_access;
u32 max_rdwr_access;
const struct bpf_ctx_arg_aux *ctx_arg_info;
- struct bpf_prog *linked_prog;
+ struct mutex tgt_mutex; /* protects writing of tgt_* pointers below */
+ struct bpf_prog *tgt_prog;
+ struct bpf_trampoline *tgt_trampoline;
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 */
bool func_proto_unreliable;
bool sleepable;
enum bpf_tramp_prog_type trampoline_prog_type;
- struct bpf_trampoline *trampoline;
struct hlist_node tramp_hlist;
/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
const struct btf_type *attach_func_proto;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2ace56c99c36..9228af9917a8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3706,7 +3706,7 @@ struct btf *btf_parse_vmlinux(void)
struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
{
- struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+ struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
if (tgt_prog) {
return tgt_prog->aux->btf;
@@ -3733,7 +3733,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
const struct btf_type *t = prog->aux->attach_func_proto;
- struct bpf_prog *tgt_prog = prog->aux->linked_prog;
+ struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
struct btf *btf = bpf_prog_get_target_btf(prog);
const char *tname = prog->aux->attach_func_name;
struct bpf_verifier_log *log = info->log;
@@ -4572,7 +4572,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
return -EFAULT;
}
if (prog_type == BPF_PROG_TYPE_EXT)
- prog_type = prog->aux->linked_prog->type;
+ prog_type = prog->aux->tgt_prog->type;
t = btf_type_by_id(btf, t->type);
if (!t || !btf_type_is_func_proto(t)) {
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..1f11484c2ec2 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -98,6 +98,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
fp->jit_requested = ebpf_jit_enabled();
INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode);
+ mutex_init(&fp->aux->tgt_mutex);
return fp;
}
@@ -253,6 +254,7 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
void __bpf_prog_free(struct bpf_prog *fp)
{
if (fp->aux) {
+ mutex_destroy(&fp->aux->tgt_mutex);
free_percpu(fp->aux->stats);
kfree(fp->aux->poke_tab);
kfree(fp->aux);
@@ -2130,7 +2132,8 @@ static void bpf_prog_free_deferred(struct work_struct *work)
if (aux->prog->has_callchain_buf)
put_callchain_buffers();
#endif
- bpf_trampoline_put(aux->trampoline);
+ if (aux->tgt_trampoline)
+ bpf_trampoline_put(aux->tgt_trampoline);
for (i = 0; i < aux->func_cnt; i++)
bpf_jit_free(aux->func[i]);
if (aux->func_cnt) {
@@ -2146,8 +2149,8 @@ void bpf_prog_free(struct bpf_prog *fp)
{
struct bpf_prog_aux *aux = fp->aux;
- if (aux->linked_prog)
- bpf_prog_put(aux->linked_prog);
+ if (aux->tgt_prog)
+ bpf_prog_put(aux->tgt_prog);
INIT_WORK(&aux->work, bpf_prog_free_deferred);
schedule_work(&aux->work);
}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4108ef3b828b..fc2bca2d9f05 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2161,7 +2161,9 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
err = PTR_ERR(tgt_prog);
goto free_prog_nouncharge;
}
- prog->aux->linked_prog = tgt_prog;
+ mutex_lock(&prog->aux->tgt_mutex);
+ prog->aux->tgt_prog = tgt_prog;
+ mutex_unlock(&prog->aux->tgt_mutex);
}
prog->aux->offload_requested = !!attr->prog_ifindex;
@@ -2498,11 +2500,22 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd)
struct bpf_tracing_link {
struct bpf_link link;
enum bpf_attach_type attach_type;
+ struct bpf_trampoline *trampoline;
+ struct bpf_prog *tgt_prog;
};
static void bpf_tracing_link_release(struct bpf_link *link)
{
- WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog));
+ struct bpf_tracing_link *tr_link =
+ container_of(link, struct bpf_tracing_link, link);
+
+ WARN_ON_ONCE(bpf_trampoline_unlink_prog(link->prog,
+ tr_link->trampoline));
+
+ bpf_trampoline_put(tr_link->trampoline);
+
+ if (tr_link->tgt_prog)
+ bpf_prog_put(tr_link->tgt_prog);
}
static void bpf_tracing_link_dealloc(struct bpf_link *link)
@@ -2545,7 +2558,9 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
static int bpf_tracing_prog_attach(struct bpf_prog *prog)
{
struct bpf_link_primer link_primer;
+ struct bpf_prog *tgt_prog = NULL;
struct bpf_tracing_link *link;
+ struct bpf_trampoline *tr;
int err;
switch (prog->type) {
@@ -2583,19 +2598,38 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
&bpf_tracing_link_lops, prog);
link->attach_type = prog->expected_attach_type;
+ mutex_lock(&prog->aux->tgt_mutex);
+
+ if (!prog->aux->tgt_trampoline) {
+ err = -ENOENT;
+ goto out_unlock;
+ }
+ tr = prog->aux->tgt_trampoline;
+ tgt_prog = prog->aux->tgt_prog;
+
err = bpf_link_prime(&link->link, &link_primer);
if (err) {
- kfree(link);
- goto out_put_prog;
+ goto out_unlock;
}
- err = bpf_trampoline_link_prog(prog);
+ err = bpf_trampoline_link_prog(prog, tr);
if (err) {
bpf_link_cleanup(&link_primer);
- goto out_put_prog;
+ link = NULL;
+ goto out_unlock;
}
+ link->tgt_prog = tgt_prog;
+ link->trampoline = tr;
+
+ prog->aux->tgt_prog = NULL;
+ prog->aux->tgt_trampoline = NULL;
+ mutex_unlock(&prog->aux->tgt_mutex);
+
return bpf_link_settle(&link_primer);
+out_unlock:
+ mutex_unlock(&prog->aux->tgt_mutex);
+ kfree(link);
out_put_prog:
bpf_prog_put(prog);
return err;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 7845913e7e41..e010a0641e99 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -261,14 +261,12 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
}
}
-int bpf_trampoline_link_prog(struct bpf_prog *prog)
+int bpf_trampoline_link_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
{
enum bpf_tramp_prog_type kind;
- struct bpf_trampoline *tr;
int err = 0;
int cnt;
- tr = prog->aux->trampoline;
kind = bpf_attach_type_to_tramp(prog);
mutex_lock(&tr->mutex);
if (tr->extension_prog) {
@@ -301,7 +299,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
}
hlist_add_head(&prog->aux->tramp_hlist, &tr->progs_hlist[kind]);
tr->progs_cnt[kind]++;
- err = bpf_trampoline_update(prog->aux->trampoline);
+ err = bpf_trampoline_update(tr);
if (err) {
hlist_del(&prog->aux->tramp_hlist);
tr->progs_cnt[kind]--;
@@ -312,13 +310,11 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
}
/* bpf_trampoline_unlink_prog() should never fail. */
-int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
+int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
{
enum bpf_tramp_prog_type kind;
- struct bpf_trampoline *tr;
int err;
- tr = prog->aux->trampoline;
kind = bpf_attach_type_to_tramp(prog);
mutex_lock(&tr->mutex);
if (kind == BPF_TRAMP_REPLACE) {
@@ -330,7 +326,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
}
hlist_del(&prog->aux->tramp_hlist);
tr->progs_cnt[kind]--;
- err = bpf_trampoline_update(prog->aux->trampoline);
+ err = bpf_trampoline_update(tr);
out:
mutex_unlock(&tr->mutex);
return err;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d38678319ca4..02f704367014 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2628,8 +2628,7 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
static enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog)
{
- return prog->aux->linked_prog ? prog->aux->linked_prog->type
- : prog->type;
+ return prog->aux->tgt_prog ? prog->aux->tgt_prog->type : prog->type;
}
static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
@@ -11271,8 +11270,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
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 bpf_prog *tgt_prog = prog->aux->tgt_prog;
struct btf_func_model fmodel;
struct bpf_trampoline *tr;
const struct btf_type *t;
@@ -11336,7 +11335,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
if (IS_ERR(tr))
return PTR_ERR(tr);
- prog->aux->trampoline = tr;
+ mutex_lock(&prog->aux->tgt_mutex);
+ prog->aux->tgt_trampoline = tr;
+ mutex_unlock(&prog->aux->tgt_mutex);
return 0;
}
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 3/8] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
2020-09-15 11:41 ` [PATCH bpf-next v5 3/8] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach Toke Høiland-Jørgensen
@ 2020-09-16 18:22 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 18:22 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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>
>
> In preparation for allowing multiple attachments of freplace programs, move
> the references to the target program and trampoline into the
> bpf_tracing_link structure when that is created. To do this atomically,
> introduce a new mutex in prog->aux to protect writing to the two pointers
> to target prog and trampoline, and rename the members to make it clear that
> they are related.
>
> With this change, it is no longer possible to attach the same tracing
> program multiple times (detaching in-between), since the reference from the
> tracing program to the target disappears on the first attach. However,
> since the next patch will let the caller supply an attach target, that will
> also make it possible to attach to the same place multiple times.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
Seems much more straightforward to me with mutex. And I don't have to
worry about various transient NULL states.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> include/linux/bpf.h | 15 +++++++++------
> kernel/bpf/btf.c | 6 +++---
> kernel/bpf/core.c | 9 ++++++---
> kernel/bpf/syscall.c | 46 ++++++++++++++++++++++++++++++++++++++++------
> kernel/bpf/trampoline.c | 12 ++++--------
> kernel/bpf/verifier.c | 9 +++++----
> 6 files changed, 67 insertions(+), 30 deletions(-)
>
[...]
> @@ -2583,19 +2598,38 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> &bpf_tracing_link_lops, prog);
> link->attach_type = prog->expected_attach_type;
>
> + mutex_lock(&prog->aux->tgt_mutex);
> +
> + if (!prog->aux->tgt_trampoline) {
> + err = -ENOENT;
> + goto out_unlock;
> + }
> + tr = prog->aux->tgt_trampoline;
> + tgt_prog = prog->aux->tgt_prog;
> +
> err = bpf_link_prime(&link->link, &link_primer);
> if (err) {
> - kfree(link);
> - goto out_put_prog;
> + goto out_unlock;
> }
nit: unnecessary {} now
>
> - err = bpf_trampoline_link_prog(prog);
> + err = bpf_trampoline_link_prog(prog, tr);
> if (err) {
> bpf_link_cleanup(&link_primer);
> - goto out_put_prog;
> + link = NULL;
> + goto out_unlock;
> }
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
` (2 preceding siblings ...)
2020-09-15 11:41 ` [PATCH bpf-next v5 3/8] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-16 19:49 ` Andrii Nakryiko
2020-09-15 11:41 ` [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
` (3 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
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 | 2 +
kernel/bpf/syscall.c | 92 ++++++++++++++++++++++++++++++++++------
kernel/bpf/verifier.c | 9 ++++
tools/include/uapi/linux/bpf.h | 2 +
5 files changed, 94 insertions(+), 13 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 939b37c78d55..360e8291e6bb 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 7dd314176df7..46eaa3024dc3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -239,6 +239,7 @@ enum bpf_attach_type {
BPF_XDP_CPUMAP,
BPF_SK_LOOKUP,
BPF_XDP,
+ BPF_TRACE_FREPLACE,
__MAX_BPF_ATTACH_TYPE
};
@@ -633,6 +634,7 @@ union bpf_attr {
__u32 flags; /* extra flags */
__aligned_u64 iter_info; /* extra bpf_iter_link_info */
__u32 iter_info_len; /* iter_info length */
+ __u32 target_btf_id; /* btf_id of target to attach to */
} link_create;
struct { /* struct used by BPF_LINK_UPDATE command */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fc2bca2d9f05..429afa820c6f 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,18 @@ 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_tracing_link *link;
+ struct btf_func_model fmodel;
+ bool new_trampoline = false;
struct bpf_trampoline *tr;
+ long addr;
+ u64 key;
int err;
switch (prog->type) {
@@ -2588,6 +2595,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
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 || !btf_id) {
+ err = -EINVAL;
+ goto out_put_prog;
+ }
+
+ tgt_prog = bpf_prog_get(tgt_prog_fd);
+ if (IS_ERR(tgt_prog)) {
+ err = PTR_ERR(tgt_prog);
+ goto out_put_prog;
+ }
+
+ key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
+ } else if (btf_id) {
+ err = -EINVAL;
+ goto out_put_prog;
+ }
link = kzalloc(sizeof(*link), GFP_USER);
if (!link) {
@@ -2600,33 +2625,65 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
mutex_lock(&prog->aux->tgt_mutex);
- if (!prog->aux->tgt_trampoline) {
- err = -ENOENT;
- goto out_unlock;
+ if (!prog->aux->tgt_trampoline ||
+ (tgt_prog && prog->aux->tgt_trampoline->key != key)) {
+ new_trampoline = true;
+ } else {
+ if (tgt_prog) {
+ /* re-using ref to tgt_prog, don't take another */
+ bpf_prog_put(tgt_prog);
+ }
+ tr = prog->aux->tgt_trampoline;
+ tgt_prog = prog->aux->tgt_prog;
+ }
+
+ if (new_trampoline) {
+ if (!tgt_prog) {
+ err = -ENOENT;
+ goto out_unlock;
+ }
+
+ err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+ &fmodel, &addr, NULL, NULL);
+ if (err) {
+ bpf_prog_put(tgt_prog);
+ goto out_unlock;
+ }
+
+ tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
+ if (IS_ERR(tr)) {
+ err = PTR_ERR(tr);
+ bpf_prog_put(tgt_prog);
+ goto out_unlock;
+ }
}
- tr = prog->aux->tgt_trampoline;
- tgt_prog = prog->aux->tgt_prog;
err = bpf_link_prime(&link->link, &link_primer);
if (err) {
- goto out_unlock;
+ goto out_put_tgt;
}
err = bpf_trampoline_link_prog(prog, tr);
if (err) {
bpf_link_cleanup(&link_primer);
link = NULL;
- goto out_unlock;
+ goto out_put_tgt;
}
link->tgt_prog = tgt_prog;
link->trampoline = tr;
-
- prog->aux->tgt_prog = NULL;
- prog->aux->tgt_trampoline = NULL;
+ if (!new_trampoline) {
+ prog->aux->tgt_trampoline = NULL;
+ prog->aux->tgt_prog = NULL;
+ }
mutex_unlock(&prog->aux->tgt_mutex);
return bpf_link_settle(&link_primer);
+out_put_tgt:
+ if (new_trampoline) {
+ bpf_prog_put(tgt_prog);
+ bpf_trampoline_put(tr);
+ }
out_unlock:
mutex_unlock(&prog->aux->tgt_mutex);
kfree(link);
@@ -2744,7 +2801,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,
@@ -2864,6 +2921,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
case BPF_CGROUP_GETSOCKOPT:
case BPF_CGROUP_SETSOCKOPT:
return BPF_PROG_TYPE_CGROUP_SOCKOPT;
+ case BPF_TRACE_FREPLACE:
+ return BPF_PROG_TYPE_EXT;
case BPF_TRACE_ITER:
return BPF_PROG_TYPE_TRACING;
case BPF_SK_LOOKUP:
@@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
prog->expected_attach_type == BPF_TRACE_ITER)
return bpf_iter_link_attach(attr, prog);
+ if (attr->link_create.attach_type == BPF_TRACE_FREPLACE &&
+ !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
+#define BPF_LINK_CREATE_LAST_FIELD link_create.target_btf_id
static int link_create(union bpf_attr *attr)
{
enum bpf_prog_type ptype;
@@ -3961,6 +4026,7 @@ static int link_create(union bpf_attr *attr)
ret = cgroup_bpf_link_attach(attr, prog);
break;
case BPF_PROG_TYPE_TRACING:
+ case BPF_PROG_TYPE_EXT:
ret = tracing_bpf_link_attach(attr, prog);
break;
case BPF_PROG_TYPE_FLOW_DISSECTOR:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 02f704367014..2dd5e2ad7f31 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 7dd314176df7..46eaa3024dc3 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -239,6 +239,7 @@ enum bpf_attach_type {
BPF_XDP_CPUMAP,
BPF_SK_LOOKUP,
BPF_XDP,
+ BPF_TRACE_FREPLACE,
__MAX_BPF_ATTACH_TYPE
};
@@ -633,6 +634,7 @@ union bpf_attr {
__u32 flags; /* extra flags */
__aligned_u64 iter_info; /* extra bpf_iter_link_info */
__u32 iter_info_len; /* iter_info length */
+ __u32 target_btf_id; /* btf_id of target to attach to */
} link_create;
struct { /* struct used by BPF_LINK_UPDATE command */
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points
2020-09-15 11:41 ` [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points Toke Høiland-Jørgensen
@ 2020-09-16 19:49 ` Andrii Nakryiko
2020-09-16 21:13 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 19:49 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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>
>
> 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 | 2 +
> kernel/bpf/syscall.c | 92 ++++++++++++++++++++++++++++++++++------
> kernel/bpf/verifier.c | 9 ++++
> tools/include/uapi/linux/bpf.h | 2 +
> 5 files changed, 94 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 939b37c78d55..360e8291e6bb 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 7dd314176df7..46eaa3024dc3 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -239,6 +239,7 @@ enum bpf_attach_type {
> BPF_XDP_CPUMAP,
> BPF_SK_LOOKUP,
> BPF_XDP,
> + BPF_TRACE_FREPLACE,
> __MAX_BPF_ATTACH_TYPE
> };
>
> @@ -633,6 +634,7 @@ union bpf_attr {
> __u32 flags; /* extra flags */
> __aligned_u64 iter_info; /* extra bpf_iter_link_info */
> __u32 iter_info_len; /* iter_info length */
> + __u32 target_btf_id; /* btf_id of target to attach to */
iter_info + iter_info_len are mutually exclusive with target_btf_id,
they should be inside a union with each other
> } link_create;
>
> struct { /* struct used by BPF_LINK_UPDATE command */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index fc2bca2d9f05..429afa820c6f 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,18 @@ 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_tracing_link *link;
> + struct btf_func_model fmodel;
> + bool new_trampoline = false;
> struct bpf_trampoline *tr;
> + long addr;
> + u64 key;
> int err;
>
> switch (prog->type) {
> @@ -2588,6 +2595,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> 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 || !btf_id) {
> + err = -EINVAL;
> + goto out_put_prog;
> + }
> +
> + tgt_prog = bpf_prog_get(tgt_prog_fd);
> + if (IS_ERR(tgt_prog)) {
> + err = PTR_ERR(tgt_prog);
> + goto out_put_prog;
> + }
> +
> + key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
> + } else if (btf_id) {
> + err = -EINVAL;
> + goto out_put_prog;
> + }
These changes are unnecessarily tangled with this if/else and
double-checking of btf_id. btf_id is required iff tgt_prog_fd is
specified, right? So just check that explicitly:
if (!!btf_id != !!targ_prog_fd)
...
bpf_iter code uses ^ instead of !=, but does same checks
if (targ_prog_fd) {
if (prog->type != BPF_PROG_TYPE_EXT)
...
} /* no else here */
More straightforward, IMO.
>
> link = kzalloc(sizeof(*link), GFP_USER);
> if (!link) {
here you are leaking tgt_prog
> @@ -2600,33 +2625,65 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>
> mutex_lock(&prog->aux->tgt_mutex);
>
> - if (!prog->aux->tgt_trampoline) {
> - err = -ENOENT;
> - goto out_unlock;
> + if (!prog->aux->tgt_trampoline ||
> + (tgt_prog && prog->aux->tgt_trampoline->key != key)) {
> + new_trampoline = true;
> + } else {
> + if (tgt_prog) {
> + /* re-using ref to tgt_prog, don't take another */
> + bpf_prog_put(tgt_prog);
this is just another complication to keep in mind, I propose to drop
it and handle at the very end, see below
> + }
> + tr = prog->aux->tgt_trampoline;
> + tgt_prog = prog->aux->tgt_prog;
> + }
this also seems too convoluted with this new_trampoline flag. Tell me
where I'm missing something.
0. get the *incorrectly* missing trampoline case (i.e., we already
attached to it and we don't specify explicit target) right out of the
way, without having nested checks:
if (!prog->aux->tgt_trampoline && !tgt_prog)
err = -ENOENT, goto;
1. Now we know that everything has to work out. new_trampoline
detection at the end is very trivial as well, so there is no need to
have this extra new_trampoline flag and split new trampoline handling
into two ifs.
if (!prog->aux->tgt_trampoline || key != tgt_trampoline->key) {
/* gotta be a new trampoline */
bpf_check_attach_target()
tr = bpf_trampoline_get()
} else {
/* reuse trampoline */
tr = ...;
tgt_prog = prog->aux->tgt_prog; /* we'll deal with refcount later */
}
> +
> + if (new_trampoline) {
> + if (!tgt_prog) {
> + err = -ENOENT;
> + goto out_unlock;
> + }
> +
> + err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
> + &fmodel, &addr, NULL, NULL);
> + if (err) {
> + bpf_prog_put(tgt_prog);
> + goto out_unlock;
> + }
> +
> + tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
> + if (IS_ERR(tr)) {
> + err = PTR_ERR(tr);
> + bpf_prog_put(tgt_prog);
> + goto out_unlock;
> + }
> }
this got rolled into a check above
> - tr = prog->aux->tgt_trampoline;
> - tgt_prog = prog->aux->tgt_prog;
>
> err = bpf_link_prime(&link->link, &link_primer);
> if (err) {
> - goto out_unlock;
> + goto out_put_tgt;
> }
>
> err = bpf_trampoline_link_prog(prog, tr);
> if (err) {
> bpf_link_cleanup(&link_primer);
> link = NULL;
> - goto out_unlock;
> + goto out_put_tgt;
> }
>
> link->tgt_prog = tgt_prog;
> link->trampoline = tr;
> -
> - prog->aux->tgt_prog = NULL;
> - prog->aux->tgt_trampoline = NULL;
> + if (!new_trampoline) {
> + prog->aux->tgt_trampoline = NULL;
> + prog->aux->tgt_prog = NULL;
> + }
this is just:
if (tr == prog->aux->tgt_trampoline) {
/* drop ref held by prog itself, we've already got tgt_prog ref from
syscall */
bpf_prog_put(prog->aux->tgt_prog);
prog->aux->tgt_prog = NULL;
prog->aux->tgt_trampoline = NULL;
}
> mutex_unlock(&prog->aux->tgt_mutex);
>
> return bpf_link_settle(&link_primer);
> +out_put_tgt:
see above, in some cases you are not putting tgt_prog where you
should. If you initialize tr to NULL, then you can do this
unconditionally on any error (no need to have different goto labels):
if (tgt_prog_fd && tgt_prog)
bpf_prog_put(tgt_prog);
if (tr && tr != prog->aux->tgt_trampoline)
bpf_trampoline_put(tr);
> + if (new_trampoline) {
> + bpf_prog_put(tgt_prog);
> + bpf_trampoline_put(tr);
> + }
> out_unlock:
> mutex_unlock(&prog->aux->tgt_mutex);
> kfree(link);
> @@ -2744,7 +2801,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,
> @@ -2864,6 +2921,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
> case BPF_CGROUP_GETSOCKOPT:
> case BPF_CGROUP_SETSOCKOPT:
> return BPF_PROG_TYPE_CGROUP_SOCKOPT;
> + case BPF_TRACE_FREPLACE:
> + return BPF_PROG_TYPE_EXT;
> case BPF_TRACE_ITER:
> return BPF_PROG_TYPE_TRACING;
> case BPF_SK_LOOKUP:
> @@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
> prog->expected_attach_type == BPF_TRACE_ITER)
> return bpf_iter_link_attach(attr, prog);
>
> + if (attr->link_create.attach_type == BPF_TRACE_FREPLACE &&
> + !prog->expected_attach_type)
> + return bpf_tracing_prog_attach(prog,
> + attr->link_create.target_fd,
> + attr->link_create.target_btf_id);
Hm.. so you added a "fake" BPF_TRACE_FREPLACE attach_type, which is
not really set with BPF_PROG_TYPE_EXT and is only specified for the
LINK_CREATE command. Are you just trying to satisfy the link_create
flow of going from attach_type to program type? If that's the only
reason, I think we can adjust link_create code to handle this more
flexibly.
I need to think a bit more whether we want BPF_TRACE_FREPLACE at all,
but if we do, whether we should make it an expected_attach_type for
BPF_PROG_TYPE_EXT then...
> +
> return -EINVAL;
> }
>
> -#define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
> +#define BPF_LINK_CREATE_LAST_FIELD link_create.target_btf_id
> static int link_create(union bpf_attr *attr)
> {
> enum bpf_prog_type ptype;
> @@ -3961,6 +4026,7 @@ static int link_create(union bpf_attr *attr)
> ret = cgroup_bpf_link_attach(attr, prog);
> break;
> case BPF_PROG_TYPE_TRACING:
> + case BPF_PROG_TYPE_EXT:
> ret = tracing_bpf_link_attach(attr, prog);
> break;
> case BPF_PROG_TYPE_FLOW_DISSECTOR:
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 02f704367014..2dd5e2ad7f31 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 7dd314176df7..46eaa3024dc3 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -239,6 +239,7 @@ enum bpf_attach_type {
> BPF_XDP_CPUMAP,
> BPF_SK_LOOKUP,
> BPF_XDP,
> + BPF_TRACE_FREPLACE,
> __MAX_BPF_ATTACH_TYPE
> };
>
> @@ -633,6 +634,7 @@ union bpf_attr {
> __u32 flags; /* extra flags */
> __aligned_u64 iter_info; /* extra bpf_iter_link_info */
> __u32 iter_info_len; /* iter_info length */
> + __u32 target_btf_id; /* btf_id of target to attach to */
> } link_create;
>
> struct { /* struct used by BPF_LINK_UPDATE command */
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points
2020-09-16 19:49 ` Andrii Nakryiko
@ 2020-09-16 21:13 ` Toke Høiland-Jørgensen
2020-09-16 21:17 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
[ will fix all your comments above ]
>> @@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
>> prog->expected_attach_type == BPF_TRACE_ITER)
>> return bpf_iter_link_attach(attr, prog);
>>
>> + if (attr->link_create.attach_type == BPF_TRACE_FREPLACE &&
>> + !prog->expected_attach_type)
>> + return bpf_tracing_prog_attach(prog,
>> + attr->link_create.target_fd,
>> + attr->link_create.target_btf_id);
>
> Hm.. so you added a "fake" BPF_TRACE_FREPLACE attach_type, which is
> not really set with BPF_PROG_TYPE_EXT and is only specified for the
> LINK_CREATE command. Are you just trying to satisfy the link_create
> flow of going from attach_type to program type? If that's the only
> reason, I think we can adjust link_create code to handle this more
> flexibly.
>
> I need to think a bit more whether we want BPF_TRACE_FREPLACE at all,
> but if we do, whether we should make it an expected_attach_type for
> BPF_PROG_TYPE_EXT then...
Yeah, wasn't too sure about this. But attach_type seemed to be the only
way to disambiguate between the different link types in the LINK_CREATE
command, so went with that. Didn't think too much about it, TBH :)
I guess an alternative could be to just enforce attach_type==0 and look
at prog->type? Or if you have any other ideas, I'm all ears!
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points
2020-09-16 21:13 ` Toke Høiland-Jørgensen
@ 2020-09-16 21:17 ` Andrii Nakryiko
2020-09-16 21:27 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 21:17 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Wed, Sep 16, 2020 at 2:13 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>
> [ will fix all your comments above ]
>
> >> @@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
> >> prog->expected_attach_type == BPF_TRACE_ITER)
> >> return bpf_iter_link_attach(attr, prog);
> >>
> >> + if (attr->link_create.attach_type == BPF_TRACE_FREPLACE &&
> >> + !prog->expected_attach_type)
> >> + return bpf_tracing_prog_attach(prog,
> >> + attr->link_create.target_fd,
> >> + attr->link_create.target_btf_id);
> >
> > Hm.. so you added a "fake" BPF_TRACE_FREPLACE attach_type, which is
> > not really set with BPF_PROG_TYPE_EXT and is only specified for the
> > LINK_CREATE command. Are you just trying to satisfy the link_create
> > flow of going from attach_type to program type? If that's the only
> > reason, I think we can adjust link_create code to handle this more
> > flexibly.
> >
> > I need to think a bit more whether we want BPF_TRACE_FREPLACE at all,
> > but if we do, whether we should make it an expected_attach_type for
> > BPF_PROG_TYPE_EXT then...
>
> Yeah, wasn't too sure about this. But attach_type seemed to be the only
> way to disambiguate between the different link types in the LINK_CREATE
> command, so went with that. Didn't think too much about it, TBH :)
having extra attach types has real costs in terms of memory (in cgroup
land), which no one ever got to fixing yet. And then
prog->expected_attach_type != link's expected_attach_type looks weird
and wrong and who knows which bugs we'll get later because of this.
>
> I guess an alternative could be to just enforce attach_type==0 and look
> at prog->type? Or if you have any other ideas, I'm all ears!
Right, we have prog fd, so can get it (regardless of type), then do
switch by type, then translate expected attach type to prog type and
see if it matches, but only for program types that care (which right
now is all but tracing, where it's obvious from prog_type alone, I
think).
>
> -Toke
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points
2020-09-16 21:17 ` Andrii Nakryiko
@ 2020-09-16 21:27 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:27 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 16, 2020 at 2:13 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>>
>> [ will fix all your comments above ]
>>
>> >> @@ -3924,10 +3983,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
>> >> prog->expected_attach_type == BPF_TRACE_ITER)
>> >> return bpf_iter_link_attach(attr, prog);
>> >>
>> >> + if (attr->link_create.attach_type == BPF_TRACE_FREPLACE &&
>> >> + !prog->expected_attach_type)
>> >> + return bpf_tracing_prog_attach(prog,
>> >> + attr->link_create.target_fd,
>> >> + attr->link_create.target_btf_id);
>> >
>> > Hm.. so you added a "fake" BPF_TRACE_FREPLACE attach_type, which is
>> > not really set with BPF_PROG_TYPE_EXT and is only specified for the
>> > LINK_CREATE command. Are you just trying to satisfy the link_create
>> > flow of going from attach_type to program type? If that's the only
>> > reason, I think we can adjust link_create code to handle this more
>> > flexibly.
>> >
>> > I need to think a bit more whether we want BPF_TRACE_FREPLACE at all,
>> > but if we do, whether we should make it an expected_attach_type for
>> > BPF_PROG_TYPE_EXT then...
>>
>> Yeah, wasn't too sure about this. But attach_type seemed to be the only
>> way to disambiguate between the different link types in the LINK_CREATE
>> command, so went with that. Didn't think too much about it, TBH :)
>
> having extra attach types has real costs in terms of memory (in cgroup
> land), which no one ever got to fixing yet. And then
> prog->expected_attach_type != link's expected_attach_type looks weird
> and wrong and who knows which bugs we'll get later because of this.
>
>>
>> I guess an alternative could be to just enforce attach_type==0 and look
>> at prog->type? Or if you have any other ideas, I'm all ears!
>
> Right, we have prog fd, so can get it (regardless of type), then do
> switch by type, then translate expected attach type to prog type and
> see if it matches, but only for program types that care (which right
> now is all but tracing, where it's obvious from prog_type alone, I
> think).
Right, makes sense; will do that in the next version!
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
` (3 preceding siblings ...)
2020-09-15 11:41 ` [PATCH bpf-next v5 4/8] bpf: support attaching freplace programs to multiple attach points Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-16 19:59 ` Andrii Nakryiko
2020-09-15 11:41 ` [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create Toke Høiland-Jørgensen
` (2 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Toke Høiland-Jørgensen <toke@redhat.com>
Eelco reported we can't properly access arguments if the tracing
program is attached to extension program.
Having following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
It's not possible to access skb argument in the fentry program,
with following error from verifier:
; int BPF_PROG(fentry, struct sk_buff *skb)
0: (79) r1 = *(u64 *)(r1 +0)
invalid bpf_context access off=0 size=8
The problem is that btf_ctx_access gets the context type for the
traced program, which is in this case the extension.
But when we trace extension program, we want to get the context
type of the program that the extension is attached to, so we can
access the argument properly in the trace program.
This version of the patch is tweaked slightly from Jiri's original one,
since the refactoring in the previous patches means we have to get the
target prog type from the new variable in prog->aux instead of directly
from the target prog.
Reported-by: Eelco Chaudron <echaudro@redhat.com>
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
kernel/bpf/btf.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 9228af9917a8..55f7b2ba1cbd 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
info->reg_type = PTR_TO_BTF_ID;
if (tgt_prog) {
- ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
+ enum bpf_prog_type tgt_type;
+
+ if (tgt_prog->type == BPF_PROG_TYPE_EXT)
+ tgt_type = tgt_prog->aux->tgt_prog_type;
+ else
+ tgt_type = tgt_prog->type;
+
+ ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
if (ret > 0) {
info->btf_id = ret;
return true;
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-15 11:41 ` [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
@ 2020-09-16 19:59 ` Andrii Nakryiko
2020-09-16 20:28 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 19:59 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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>
>
> Eelco reported we can't properly access arguments if the tracing
> program is attached to extension program.
>
> Having following program:
>
> SEC("classifier/test_pkt_md_access")
> int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
> SEC("freplace/test_pkt_md_access")
> int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
> SEC("fentry/test_pkt_md_access_new")
> int BPF_PROG(fentry, struct sk_buff *skb)
>
> It's not possible to access skb argument in the fentry program,
> with following error from verifier:
>
> ; int BPF_PROG(fentry, struct sk_buff *skb)
> 0: (79) r1 = *(u64 *)(r1 +0)
> invalid bpf_context access off=0 size=8
>
> The problem is that btf_ctx_access gets the context type for the
> traced program, which is in this case the extension.
>
> But when we trace extension program, we want to get the context
> type of the program that the extension is attached to, so we can
> access the argument properly in the trace program.
>
> This version of the patch is tweaked slightly from Jiri's original one,
> since the refactoring in the previous patches means we have to get the
> target prog type from the new variable in prog->aux instead of directly
> from the target prog.
>
> Reported-by: Eelco Chaudron <echaudro@redhat.com>
> Suggested-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> kernel/bpf/btf.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 9228af9917a8..55f7b2ba1cbd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>
> info->reg_type = PTR_TO_BTF_ID;
> if (tgt_prog) {
> - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> + enum bpf_prog_type tgt_type;
> +
> + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> + tgt_type = tgt_prog->aux->tgt_prog_type;
what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
this be a loop?
Which also brings up a few follow up questions. Now that we allow same
PROG_EXT program to be attached to multiple other programs:
1. what prevents us from attaching PROG_EXT to itself?
2. How do we prevent long chain of EXT programs or even loops?
Can you please add a few selftests testing such cases? I have a
feeling that with your changes in this patch set now it's possible to
break the kernel very easily. I don't know what the proper solution
is, but let's at least have a test that does show breakage, then try
to figure out the solution. See also comment in check_attach_btf_id()
about fentry/fexit and freplace interactions. That might not be
enough.
> + else
> + tgt_type = tgt_prog->type;
> +
> + ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
> if (ret > 0) {
> info->btf_id = ret;
> return true;
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-16 19:59 ` Andrii Nakryiko
@ 2020-09-16 20:28 ` Andrii Nakryiko
2020-09-16 21:15 ` Toke Høiland-Jørgensen
2020-09-17 17:10 ` Toke Høiland-Jørgensen
0 siblings, 2 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 20:28 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> 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>
> >
> > Eelco reported we can't properly access arguments if the tracing
> > program is attached to extension program.
> >
> > Having following program:
> >
> > SEC("classifier/test_pkt_md_access")
> > int test_pkt_md_access(struct __sk_buff *skb)
> >
> > with its extension:
> >
> > SEC("freplace/test_pkt_md_access")
> > int test_pkt_md_access_new(struct __sk_buff *skb)
> >
> > and tracing that extension with:
> >
> > SEC("fentry/test_pkt_md_access_new")
> > int BPF_PROG(fentry, struct sk_buff *skb)
> >
> > It's not possible to access skb argument in the fentry program,
> > with following error from verifier:
> >
> > ; int BPF_PROG(fentry, struct sk_buff *skb)
> > 0: (79) r1 = *(u64 *)(r1 +0)
> > invalid bpf_context access off=0 size=8
> >
> > The problem is that btf_ctx_access gets the context type for the
> > traced program, which is in this case the extension.
> >
> > But when we trace extension program, we want to get the context
> > type of the program that the extension is attached to, so we can
> > access the argument properly in the trace program.
> >
> > This version of the patch is tweaked slightly from Jiri's original one,
> > since the refactoring in the previous patches means we have to get the
> > target prog type from the new variable in prog->aux instead of directly
> > from the target prog.
> >
> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> > kernel/bpf/btf.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 9228af9917a8..55f7b2ba1cbd 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >
> > info->reg_type = PTR_TO_BTF_ID;
> > if (tgt_prog) {
> > - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> > + enum bpf_prog_type tgt_type;
> > +
> > + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> > + tgt_type = tgt_prog->aux->tgt_prog_type;
>
> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
> this be a loop?
ok, never mind this specifically. there is an explicit check
if (tgt_prog->type == prog->type) {
verbose(env, "Cannot recursively attach\n");
return -EINVAL;
}
that will prevent this.
But, I think we still will be able to construct a long chain of
fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
infinitum. Can you please construct such a selftest? And then we
should probably fix those checks to also disallow FMOD_RET, in
addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
prog type should check if that can cause any problems).
>
> Which also brings up a few follow up questions. Now that we allow same
> PROG_EXT program to be attached to multiple other programs:
>
> 1. what prevents us from attaching PROG_EXT to itself?
> 2. How do we prevent long chain of EXT programs or even loops?
>
> Can you please add a few selftests testing such cases? I have a
> feeling that with your changes in this patch set now it's possible to
> break the kernel very easily. I don't know what the proper solution
> is, but let's at least have a test that does show breakage, then try
> to figure out the solution. See also comment in check_attach_btf_id()
> about fentry/fexit and freplace interactions. That might not be
> enough.
>
>
> > + else
> > + tgt_type = tgt_prog->type;
> > +
> > + ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
> > if (ret > 0) {
> > info->btf_id = ret;
> > return true;
> >
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-16 20:28 ` Andrii Nakryiko
@ 2020-09-16 21:15 ` Toke Høiland-Jørgensen
2020-09-17 17:10 ` Toke Høiland-Jørgensen
1 sibling, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:15 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> 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>
>> >
>> > Eelco reported we can't properly access arguments if the tracing
>> > program is attached to extension program.
>> >
>> > Having following program:
>> >
>> > SEC("classifier/test_pkt_md_access")
>> > int test_pkt_md_access(struct __sk_buff *skb)
>> >
>> > with its extension:
>> >
>> > SEC("freplace/test_pkt_md_access")
>> > int test_pkt_md_access_new(struct __sk_buff *skb)
>> >
>> > and tracing that extension with:
>> >
>> > SEC("fentry/test_pkt_md_access_new")
>> > int BPF_PROG(fentry, struct sk_buff *skb)
>> >
>> > It's not possible to access skb argument in the fentry program,
>> > with following error from verifier:
>> >
>> > ; int BPF_PROG(fentry, struct sk_buff *skb)
>> > 0: (79) r1 = *(u64 *)(r1 +0)
>> > invalid bpf_context access off=0 size=8
>> >
>> > The problem is that btf_ctx_access gets the context type for the
>> > traced program, which is in this case the extension.
>> >
>> > But when we trace extension program, we want to get the context
>> > type of the program that the extension is attached to, so we can
>> > access the argument properly in the trace program.
>> >
>> > This version of the patch is tweaked slightly from Jiri's original one,
>> > since the refactoring in the previous patches means we have to get the
>> > target prog type from the new variable in prog->aux instead of directly
>> > from the target prog.
>> >
>> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> > kernel/bpf/btf.c | 9 ++++++++-
>> > 1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> > index 9228af9917a8..55f7b2ba1cbd 100644
>> > --- a/kernel/bpf/btf.c
>> > +++ b/kernel/bpf/btf.c
>> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >
>> > info->reg_type = PTR_TO_BTF_ID;
>> > if (tgt_prog) {
>> > - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> > + enum bpf_prog_type tgt_type;
>> > +
>> > + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> > + tgt_type = tgt_prog->aux->tgt_prog_type;
>>
>> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> this be a loop?
>
> ok, never mind this specifically. there is an explicit check
>
> if (tgt_prog->type == prog->type) {
> verbose(env, "Cannot recursively attach\n");
> return -EINVAL;
> }
>
> that will prevent this.
>
> But, I think we still will be able to construct a long chain of
> fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> infinitum. Can you please construct such a selftest? And then we
> should probably fix those checks to also disallow FMOD_RET, in
> addition to BPF_TRACE_FENTRY/FEXIT
Sure, can do!
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-16 20:28 ` Andrii Nakryiko
2020-09-16 21:15 ` Toke Høiland-Jørgensen
@ 2020-09-17 17:10 ` Toke Høiland-Jørgensen
2020-09-17 18:06 ` Andrii Nakryiko
1 sibling, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-17 17:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> 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>
>> >
>> > Eelco reported we can't properly access arguments if the tracing
>> > program is attached to extension program.
>> >
>> > Having following program:
>> >
>> > SEC("classifier/test_pkt_md_access")
>> > int test_pkt_md_access(struct __sk_buff *skb)
>> >
>> > with its extension:
>> >
>> > SEC("freplace/test_pkt_md_access")
>> > int test_pkt_md_access_new(struct __sk_buff *skb)
>> >
>> > and tracing that extension with:
>> >
>> > SEC("fentry/test_pkt_md_access_new")
>> > int BPF_PROG(fentry, struct sk_buff *skb)
>> >
>> > It's not possible to access skb argument in the fentry program,
>> > with following error from verifier:
>> >
>> > ; int BPF_PROG(fentry, struct sk_buff *skb)
>> > 0: (79) r1 = *(u64 *)(r1 +0)
>> > invalid bpf_context access off=0 size=8
>> >
>> > The problem is that btf_ctx_access gets the context type for the
>> > traced program, which is in this case the extension.
>> >
>> > But when we trace extension program, we want to get the context
>> > type of the program that the extension is attached to, so we can
>> > access the argument properly in the trace program.
>> >
>> > This version of the patch is tweaked slightly from Jiri's original one,
>> > since the refactoring in the previous patches means we have to get the
>> > target prog type from the new variable in prog->aux instead of directly
>> > from the target prog.
>> >
>> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> > kernel/bpf/btf.c | 9 ++++++++-
>> > 1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> > index 9228af9917a8..55f7b2ba1cbd 100644
>> > --- a/kernel/bpf/btf.c
>> > +++ b/kernel/bpf/btf.c
>> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >
>> > info->reg_type = PTR_TO_BTF_ID;
>> > if (tgt_prog) {
>> > - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> > + enum bpf_prog_type tgt_type;
>> > +
>> > + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> > + tgt_type = tgt_prog->aux->tgt_prog_type;
>>
>> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> this be a loop?
>
> ok, never mind this specifically. there is an explicit check
>
> if (tgt_prog->type == prog->type) {
> verbose(env, "Cannot recursively attach\n");
> return -EINVAL;
> }
>
> that will prevent this.
>
> But, I think we still will be able to construct a long chain of
> fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> infinitum. Can you please construct such a selftest? And then we
> should probably fix those checks to also disallow FMOD_RET, in
> addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
> prog type should check if that can cause any problems).
Huh, I thought fmod_ret was supposed to be for kernel functions only?
However, I can't really point to anywhere in the code that ensures this,
other than check_attach_modify_return(), but I think that will allow a
bpf function as long as its name starts with "security_" ?
Is there actually any use case for modify_return being attached to a BPF
function (you could just use freplace instead, couldn't you?). Or should
we just disallow that entirely (if I'm not missing somewhere it's
already blocked)?
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-17 17:10 ` Toke Høiland-Jørgensen
@ 2020-09-17 18:06 ` Andrii Nakryiko
2020-09-17 18:44 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-17 18:06 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Thu, Sep 17, 2020 at 10:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> 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>
> >> >
> >> > Eelco reported we can't properly access arguments if the tracing
> >> > program is attached to extension program.
> >> >
> >> > Having following program:
> >> >
> >> > SEC("classifier/test_pkt_md_access")
> >> > int test_pkt_md_access(struct __sk_buff *skb)
> >> >
> >> > with its extension:
> >> >
> >> > SEC("freplace/test_pkt_md_access")
> >> > int test_pkt_md_access_new(struct __sk_buff *skb)
> >> >
> >> > and tracing that extension with:
> >> >
> >> > SEC("fentry/test_pkt_md_access_new")
> >> > int BPF_PROG(fentry, struct sk_buff *skb)
> >> >
> >> > It's not possible to access skb argument in the fentry program,
> >> > with following error from verifier:
> >> >
> >> > ; int BPF_PROG(fentry, struct sk_buff *skb)
> >> > 0: (79) r1 = *(u64 *)(r1 +0)
> >> > invalid bpf_context access off=0 size=8
> >> >
> >> > The problem is that btf_ctx_access gets the context type for the
> >> > traced program, which is in this case the extension.
> >> >
> >> > But when we trace extension program, we want to get the context
> >> > type of the program that the extension is attached to, so we can
> >> > access the argument properly in the trace program.
> >> >
> >> > This version of the patch is tweaked slightly from Jiri's original one,
> >> > since the refactoring in the previous patches means we have to get the
> >> > target prog type from the new variable in prog->aux instead of directly
> >> > from the target prog.
> >> >
> >> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
> >> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> > ---
> >> > kernel/bpf/btf.c | 9 ++++++++-
> >> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >> > index 9228af9917a8..55f7b2ba1cbd 100644
> >> > --- a/kernel/bpf/btf.c
> >> > +++ b/kernel/bpf/btf.c
> >> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >> >
> >> > info->reg_type = PTR_TO_BTF_ID;
> >> > if (tgt_prog) {
> >> > - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> >> > + enum bpf_prog_type tgt_type;
> >> > +
> >> > + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> >> > + tgt_type = tgt_prog->aux->tgt_prog_type;
> >>
> >> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
> >> this be a loop?
> >
> > ok, never mind this specifically. there is an explicit check
> >
> > if (tgt_prog->type == prog->type) {
> > verbose(env, "Cannot recursively attach\n");
> > return -EINVAL;
> > }
> >
> > that will prevent this.
> >
> > But, I think we still will be able to construct a long chain of
> > fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> > infinitum. Can you please construct such a selftest? And then we
> > should probably fix those checks to also disallow FMOD_RET, in
> > addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
> > prog type should check if that can cause any problems).
>
> Huh, I thought fmod_ret was supposed to be for kernel functions only?
Yeah, I realized that afterwards, but didn't want to ramble on forever :)
> However, I can't really point to anywhere in the code that ensures this,
> other than check_attach_modify_return(), but I think that will allow a
> bpf function as long as its name starts with "security_" ?
I think error_injection_list check will disallow anything that's not a
specially marked kernel function. So we are probably safe as is, even
though a bit implicitly.
>
> Is there actually any use case for modify_return being attached to a BPF
> function (you could just use freplace instead, couldn't you?). Or should
> we just disallow that entirely (if I'm not missing somewhere it's
> already blocked)?
No idea, but I think it works as is right now, so I wouldn't touch it.
>
> -Toke
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs
2020-09-17 18:06 ` Andrii Nakryiko
@ 2020-09-17 18:44 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-17 18:44 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Thu, Sep 17, 2020 at 10:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> >>
>> >> 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>
>> >> >
>> >> > Eelco reported we can't properly access arguments if the tracing
>> >> > program is attached to extension program.
>> >> >
>> >> > Having following program:
>> >> >
>> >> > SEC("classifier/test_pkt_md_access")
>> >> > int test_pkt_md_access(struct __sk_buff *skb)
>> >> >
>> >> > with its extension:
>> >> >
>> >> > SEC("freplace/test_pkt_md_access")
>> >> > int test_pkt_md_access_new(struct __sk_buff *skb)
>> >> >
>> >> > and tracing that extension with:
>> >> >
>> >> > SEC("fentry/test_pkt_md_access_new")
>> >> > int BPF_PROG(fentry, struct sk_buff *skb)
>> >> >
>> >> > It's not possible to access skb argument in the fentry program,
>> >> > with following error from verifier:
>> >> >
>> >> > ; int BPF_PROG(fentry, struct sk_buff *skb)
>> >> > 0: (79) r1 = *(u64 *)(r1 +0)
>> >> > invalid bpf_context access off=0 size=8
>> >> >
>> >> > The problem is that btf_ctx_access gets the context type for the
>> >> > traced program, which is in this case the extension.
>> >> >
>> >> > But when we trace extension program, we want to get the context
>> >> > type of the program that the extension is attached to, so we can
>> >> > access the argument properly in the trace program.
>> >> >
>> >> > This version of the patch is tweaked slightly from Jiri's original one,
>> >> > since the refactoring in the previous patches means we have to get the
>> >> > target prog type from the new variable in prog->aux instead of directly
>> >> > from the target prog.
>> >> >
>> >> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> >> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> > ---
>> >> > kernel/bpf/btf.c | 9 ++++++++-
>> >> > 1 file changed, 8 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> >> > index 9228af9917a8..55f7b2ba1cbd 100644
>> >> > --- a/kernel/bpf/btf.c
>> >> > +++ b/kernel/bpf/btf.c
>> >> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >> >
>> >> > info->reg_type = PTR_TO_BTF_ID;
>> >> > if (tgt_prog) {
>> >> > - ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> >> > + enum bpf_prog_type tgt_type;
>> >> > +
>> >> > + if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> >> > + tgt_type = tgt_prog->aux->tgt_prog_type;
>> >>
>> >> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> >> this be a loop?
>> >
>> > ok, never mind this specifically. there is an explicit check
>> >
>> > if (tgt_prog->type == prog->type) {
>> > verbose(env, "Cannot recursively attach\n");
>> > return -EINVAL;
>> > }
>> >
>> > that will prevent this.
>> >
>> > But, I think we still will be able to construct a long chain of
>> > fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
>> > infinitum. Can you please construct such a selftest? And then we
>> > should probably fix those checks to also disallow FMOD_RET, in
>> > addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
>> > prog type should check if that can cause any problems).
>>
>> Huh, I thought fmod_ret was supposed to be for kernel functions only?
>
> Yeah, I realized that afterwards, but didn't want to ramble on forever :)
>
>> However, I can't really point to anywhere in the code that ensures this,
>> other than check_attach_modify_return(), but I think that will allow a
>> bpf function as long as its name starts with "security_" ?
>
> I think error_injection_list check will disallow anything that's not a
> specially marked kernel function. So we are probably safe as is, even
> though a bit implicitly.
Got a selftest working now, and no, it seems not. At least attachment
will succeed if the freplace program has a security_ prefix in its
function name. So will add a new patch to fix that, and the selftest :)
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
` (4 preceding siblings ...)
2020-09-15 11:41 ` [PATCH bpf-next v5 5/8] bpf: Fix context type resolving for extension programs Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-16 20:37 ` Andrii Nakryiko
2020-09-15 11:41 ` [PATCH bpf-next v5 7/8] selftests: add test for multiple attachments of freplace program Toke Høiland-Jørgensen
2020-09-15 11:41 ` [PATCH bpf-next v5 8/8] selftests/bpf: Adding test for arg dereference in extension trace Toke Høiland-Jørgensen
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Toke Høiland-Jørgensen <toke@redhat.com>
This adds support for supplying a target btf ID for the bpf_link_create()
operation, and adds a new bpf_program__attach_freplace() high-level API for
attaching freplace functions with a target.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
tools/lib/bpf/bpf.c | 1 +
tools/lib/bpf/bpf.h | 3 ++-
tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
tools/lib/bpf/libbpf.h | 3 +++
tools/lib/bpf/libbpf.map | 1 +
5 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 82b983ff6569..e928456c0dd6 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
attr.link_create.iter_info =
ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
+ attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
}
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 015d13f25fcc..f8dbf666b62b 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -174,8 +174,9 @@ struct bpf_link_create_opts {
__u32 flags;
union bpf_iter_link_info *iter_info;
__u32 iter_info_len;
+ __u32 target_btf_id;
};
-#define bpf_link_create_opts__last_field iter_info_len
+#define bpf_link_create_opts__last_field target_btf_id
LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
enum bpf_attach_type attach_type,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 550950eb1860..165131c73f40 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
static struct bpf_link *
bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
- const char *target_name)
+ int target_btf_id, const char *target_name)
{
enum bpf_attach_type attach_type;
char errmsg[STRERR_BUFSIZE];
struct bpf_link *link;
int prog_fd, link_fd;
+ DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
+ .target_btf_id = target_btf_id);
prog_fd = bpf_program__fd(prog);
if (prog_fd < 0) {
@@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
return ERR_PTR(-ENOMEM);
link->detach = &bpf_link__detach_fd;
- attach_type = bpf_program__get_expected_attach_type(prog);
- link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
+ if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
+ attach_type = BPF_TRACE_FREPLACE;
+ else
+ attach_type = bpf_program__get_expected_attach_type(prog);
+
+ link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
if (link_fd < 0) {
link_fd = -errno;
free(link);
@@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
struct bpf_link *
bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
{
- return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
+ return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
}
struct bpf_link *
bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
{
- return bpf_program__attach_fd(prog, netns_fd, "netns");
+ return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
}
struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
{
/* target_fd/target_ifindex use the same field in LINK_CREATE */
- return bpf_program__attach_fd(prog, ifindex, "xdp");
+ return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
+}
+
+struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
+ int target_fd, int target_btf_id)
+{
+ return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace");
}
struct bpf_link *
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index a750f67a23f6..ce5add9b9203 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
LIBBPF_API struct bpf_link *
bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_freplace(struct bpf_program *prog,
+ int target_fd, int target_btf_id);
struct bpf_map;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 92ceb48a5ca2..3cc2c42f435d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -302,6 +302,7 @@ LIBBPF_0.1.0 {
LIBBPF_0.2.0 {
global:
+ bpf_program__attach_freplace;
bpf_program__section_name;
perf_buffer__buffer_cnt;
perf_buffer__buffer_fd;
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-15 11:41 ` [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create Toke Høiland-Jørgensen
@ 2020-09-16 20:37 ` Andrii Nakryiko
2020-09-16 20:45 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 20:37 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
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>
>
> This adds support for supplying a target btf ID for the bpf_link_create()
> operation, and adds a new bpf_program__attach_freplace() high-level API for
> attaching freplace functions with a target.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> tools/lib/bpf/bpf.c | 1 +
> tools/lib/bpf/bpf.h | 3 ++-
> tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
> tools/lib/bpf/libbpf.h | 3 +++
> tools/lib/bpf/libbpf.map | 1 +
> 5 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 82b983ff6569..e928456c0dd6 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
> attr.link_create.iter_info =
> ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
> attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
> + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
>
> return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
> }
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 015d13f25fcc..f8dbf666b62b 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -174,8 +174,9 @@ struct bpf_link_create_opts {
> __u32 flags;
> union bpf_iter_link_info *iter_info;
> __u32 iter_info_len;
> + __u32 target_btf_id;
> };
> -#define bpf_link_create_opts__last_field iter_info_len
> +#define bpf_link_create_opts__last_field target_btf_id
>
> LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
> enum bpf_attach_type attach_type,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 550950eb1860..165131c73f40 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
>
> static struct bpf_link *
> bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> - const char *target_name)
> + int target_btf_id, const char *target_name)
> {
> enum bpf_attach_type attach_type;
> char errmsg[STRERR_BUFSIZE];
> struct bpf_link *link;
> int prog_fd, link_fd;
> + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> + .target_btf_id = target_btf_id);
>
> prog_fd = bpf_program__fd(prog);
> if (prog_fd < 0) {
> @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> return ERR_PTR(-ENOMEM);
> link->detach = &bpf_link__detach_fd;
>
> - attach_type = bpf_program__get_expected_attach_type(prog);
> - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
> + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
> + attach_type = BPF_TRACE_FREPLACE;
doing this unconditionally will break an old-style freplace without
target_fd/btf_id on older kernels. Safe and simple way would be to
continue using raw_tracepoint_open when there is no target_fd/btf_id,
and use LINK_CREATE for newer stuff. Alternatively, you'd need to do
feature detection, but it's still would be nice to handle old-style
attach through raw_tracepoint_open for bpf_program__attach_freplace.
so I suggest leaving bpf_program__attach_fd() as is and to create a
custom bpf_program__attach_freplace() implementation.
> + else
> + attach_type = bpf_program__get_expected_attach_type(prog);
> +
> + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
> if (link_fd < 0) {
> link_fd = -errno;
> free(link);
> @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> struct bpf_link *
> bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
> {
> - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
> + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
> }
>
> struct bpf_link *
> bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
> {
> - return bpf_program__attach_fd(prog, netns_fd, "netns");
> + return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
> }
>
> struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
> {
> /* target_fd/target_ifindex use the same field in LINK_CREATE */
> - return bpf_program__attach_fd(prog, ifindex, "xdp");
> + return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
> +}
> +
> +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
> + int target_fd, int target_btf_id)
> +{
> + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace");
> }
>
> struct bpf_link *
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index a750f67a23f6..ce5add9b9203 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
> bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
> LIBBPF_API struct bpf_link *
> bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_freplace(struct bpf_program *prog,
> + int target_fd, int target_btf_id);
maybe a const char * function name instead of target_btf_id would be a
nicer API? Users won't have to deal with fetching target prog's BTF,
searching it, etc. That's all pretty straightforward for libbpf to do,
leaving users with more natural and simpler API.
>
> struct bpf_map;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 92ceb48a5ca2..3cc2c42f435d 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -302,6 +302,7 @@ LIBBPF_0.1.0 {
>
> LIBBPF_0.2.0 {
> global:
> + bpf_program__attach_freplace;
> bpf_program__section_name;
> perf_buffer__buffer_cnt;
> perf_buffer__buffer_fd;
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-16 20:37 ` Andrii Nakryiko
@ 2020-09-16 20:45 ` Andrii Nakryiko
2020-09-16 21:21 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 20:45 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> 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>
> >
> > This adds support for supplying a target btf ID for the bpf_link_create()
> > operation, and adds a new bpf_program__attach_freplace() high-level API for
> > attaching freplace functions with a target.
> >
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> > tools/lib/bpf/bpf.c | 1 +
> > tools/lib/bpf/bpf.h | 3 ++-
> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
> > tools/lib/bpf/libbpf.h | 3 +++
> > tools/lib/bpf/libbpf.map | 1 +
> > 5 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 82b983ff6569..e928456c0dd6 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
> > attr.link_create.iter_info =
> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
> >
> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
> > }
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 015d13f25fcc..f8dbf666b62b 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts {
> > __u32 flags;
> > union bpf_iter_link_info *iter_info;
> > __u32 iter_info_len;
> > + __u32 target_btf_id;
> > };
> > -#define bpf_link_create_opts__last_field iter_info_len
> > +#define bpf_link_create_opts__last_field target_btf_id
> >
> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
> > enum bpf_attach_type attach_type,
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 550950eb1860..165131c73f40 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
> >
> > static struct bpf_link *
> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> > - const char *target_name)
> > + int target_btf_id, const char *target_name)
> > {
> > enum bpf_attach_type attach_type;
> > char errmsg[STRERR_BUFSIZE];
> > struct bpf_link *link;
> > int prog_fd, link_fd;
> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> > + .target_btf_id = target_btf_id);
> >
> > prog_fd = bpf_program__fd(prog);
> > if (prog_fd < 0) {
> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> > return ERR_PTR(-ENOMEM);
> > link->detach = &bpf_link__detach_fd;
> >
> > - attach_type = bpf_program__get_expected_attach_type(prog);
> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
> > + attach_type = BPF_TRACE_FREPLACE;
>
> doing this unconditionally will break an old-style freplace without
> target_fd/btf_id on older kernels. Safe and simple way would be to
> continue using raw_tracepoint_open when there is no target_fd/btf_id,
> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do
> feature detection, but it's still would be nice to handle old-style
> attach through raw_tracepoint_open for bpf_program__attach_freplace.
>
> so I suggest leaving bpf_program__attach_fd() as is and to create a
> custom bpf_program__attach_freplace() implementation.
>
> > + else
> > + attach_type = bpf_program__get_expected_attach_type(prog);
> > +
> > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
> > if (link_fd < 0) {
> > link_fd = -errno;
> > free(link);
> > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> > struct bpf_link *
> > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
> > {
> > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
> > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
> > }
> >
> > struct bpf_link *
> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
> > {
> > - return bpf_program__attach_fd(prog, netns_fd, "netns");
> > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
> > }
> >
> > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
> > {
> > /* target_fd/target_ifindex use the same field in LINK_CREATE */
> > - return bpf_program__attach_fd(prog, ifindex, "xdp");
> > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
> > + int target_fd, int target_btf_id)
> > +{
> > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace");
> > }
> >
> > struct bpf_link *
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index a750f67a23f6..ce5add9b9203 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
> > LIBBPF_API struct bpf_link *
> > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_freplace(struct bpf_program *prog,
> > + int target_fd, int target_btf_id);
>
> maybe a const char * function name instead of target_btf_id would be a
> nicer API? Users won't have to deal with fetching target prog's BTF,
> searching it, etc. That's all pretty straightforward for libbpf to do,
> leaving users with more natural and simpler API.
>
bpf_program__set_attach_target() uses string name for target
functions, we should definitely be consistent here
> >
> > struct bpf_map;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 92ceb48a5ca2..3cc2c42f435d 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -302,6 +302,7 @@ LIBBPF_0.1.0 {
> >
> > LIBBPF_0.2.0 {
> > global:
> > + bpf_program__attach_freplace;
> > bpf_program__section_name;
> > perf_buffer__buffer_cnt;
> > perf_buffer__buffer_fd;
> >
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-16 20:45 ` Andrii Nakryiko
@ 2020-09-16 21:21 ` Toke Høiland-Jørgensen
2020-09-16 21:24 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> 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>
>> >
>> > This adds support for supplying a target btf ID for the bpf_link_create()
>> > operation, and adds a new bpf_program__attach_freplace() high-level API for
>> > attaching freplace functions with a target.
>> >
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> > tools/lib/bpf/bpf.c | 1 +
>> > tools/lib/bpf/bpf.h | 3 ++-
>> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
>> > tools/lib/bpf/libbpf.h | 3 +++
>> > tools/lib/bpf/libbpf.map | 1 +
>> > 5 files changed, 25 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> > index 82b983ff6569..e928456c0dd6 100644
>> > --- a/tools/lib/bpf/bpf.c
>> > +++ b/tools/lib/bpf/bpf.c
>> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
>> > attr.link_create.iter_info =
>> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
>> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
>> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
>> >
>> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>> > }
>> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> > index 015d13f25fcc..f8dbf666b62b 100644
>> > --- a/tools/lib/bpf/bpf.h
>> > +++ b/tools/lib/bpf/bpf.h
>> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts {
>> > __u32 flags;
>> > union bpf_iter_link_info *iter_info;
>> > __u32 iter_info_len;
>> > + __u32 target_btf_id;
>> > };
>> > -#define bpf_link_create_opts__last_field iter_info_len
>> > +#define bpf_link_create_opts__last_field target_btf_id
>> >
>> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>> > enum bpf_attach_type attach_type,
>> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> > index 550950eb1860..165131c73f40 100644
>> > --- a/tools/lib/bpf/libbpf.c
>> > +++ b/tools/lib/bpf/libbpf.c
>> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
>> >
>> > static struct bpf_link *
>> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
>> > - const char *target_name)
>> > + int target_btf_id, const char *target_name)
>> > {
>> > enum bpf_attach_type attach_type;
>> > char errmsg[STRERR_BUFSIZE];
>> > struct bpf_link *link;
>> > int prog_fd, link_fd;
>> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
>> > + .target_btf_id = target_btf_id);
>> >
>> > prog_fd = bpf_program__fd(prog);
>> > if (prog_fd < 0) {
>> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
>> > return ERR_PTR(-ENOMEM);
>> > link->detach = &bpf_link__detach_fd;
>> >
>> > - attach_type = bpf_program__get_expected_attach_type(prog);
>> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
>> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
>> > + attach_type = BPF_TRACE_FREPLACE;
>>
>> doing this unconditionally will break an old-style freplace without
>> target_fd/btf_id on older kernels. Safe and simple way would be to
>> continue using raw_tracepoint_open when there is no target_fd/btf_id,
>> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do
>> feature detection, but it's still would be nice to handle old-style
>> attach through raw_tracepoint_open for bpf_program__attach_freplace.
>>
>> so I suggest leaving bpf_program__attach_fd() as is and to create a
>> custom bpf_program__attach_freplace() implementation.
Sure, I'll take another pass at this. Not sure how useful feature
detection in libbpf is; if the caller passes a target, libbpf can't
really do much if the kernel doesn't support it...
>> > + else
>> > + attach_type = bpf_program__get_expected_attach_type(prog);
>> > +
>> > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
>> > if (link_fd < 0) {
>> > link_fd = -errno;
>> > free(link);
>> > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
>> > struct bpf_link *
>> > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
>> > {
>> > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
>> > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
>> > }
>> >
>> > struct bpf_link *
>> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
>> > {
>> > - return bpf_program__attach_fd(prog, netns_fd, "netns");
>> > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
>> > }
>> >
>> > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
>> > {
>> > /* target_fd/target_ifindex use the same field in LINK_CREATE */
>> > - return bpf_program__attach_fd(prog, ifindex, "xdp");
>> > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
>> > +}
>> > +
>> > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
>> > + int target_fd, int target_btf_id)
>> > +{
>> > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace");
>> > }
>> >
>> > struct bpf_link *
>> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> > index a750f67a23f6..ce5add9b9203 100644
>> > --- a/tools/lib/bpf/libbpf.h
>> > +++ b/tools/lib/bpf/libbpf.h
>> > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
>> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
>> > LIBBPF_API struct bpf_link *
>> > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
>> > +LIBBPF_API struct bpf_link *
>> > +bpf_program__attach_freplace(struct bpf_program *prog,
>> > + int target_fd, int target_btf_id);
>>
>> maybe a const char * function name instead of target_btf_id would be a
>> nicer API? Users won't have to deal with fetching target prog's BTF,
>> searching it, etc. That's all pretty straightforward for libbpf to do,
>> leaving users with more natural and simpler API.
>>
>
> bpf_program__set_attach_target() uses string name for target
> functions, we should definitely be consistent here
All right, fair enough :)
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-16 21:21 ` Toke Høiland-Jørgensen
@ 2020-09-16 21:24 ` Andrii Nakryiko
2020-09-16 21:41 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 21:24 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Wed, Sep 16, 2020 at 2:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> 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>
> >> >
> >> > This adds support for supplying a target btf ID for the bpf_link_create()
> >> > operation, and adds a new bpf_program__attach_freplace() high-level API for
> >> > attaching freplace functions with a target.
> >> >
> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> > ---
> >> > tools/lib/bpf/bpf.c | 1 +
> >> > tools/lib/bpf/bpf.h | 3 ++-
> >> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
> >> > tools/lib/bpf/libbpf.h | 3 +++
> >> > tools/lib/bpf/libbpf.map | 1 +
> >> > 5 files changed, 25 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >> > index 82b983ff6569..e928456c0dd6 100644
> >> > --- a/tools/lib/bpf/bpf.c
> >> > +++ b/tools/lib/bpf/bpf.c
> >> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
> >> > attr.link_create.iter_info =
> >> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
> >> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
> >> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
> >> >
> >> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
> >> > }
> >> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> >> > index 015d13f25fcc..f8dbf666b62b 100644
> >> > --- a/tools/lib/bpf/bpf.h
> >> > +++ b/tools/lib/bpf/bpf.h
> >> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts {
> >> > __u32 flags;
> >> > union bpf_iter_link_info *iter_info;
> >> > __u32 iter_info_len;
> >> > + __u32 target_btf_id;
> >> > };
> >> > -#define bpf_link_create_opts__last_field iter_info_len
> >> > +#define bpf_link_create_opts__last_field target_btf_id
> >> >
> >> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
> >> > enum bpf_attach_type attach_type,
> >> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> > index 550950eb1860..165131c73f40 100644
> >> > --- a/tools/lib/bpf/libbpf.c
> >> > +++ b/tools/lib/bpf/libbpf.c
> >> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
> >> >
> >> > static struct bpf_link *
> >> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> >> > - const char *target_name)
> >> > + int target_btf_id, const char *target_name)
> >> > {
> >> > enum bpf_attach_type attach_type;
> >> > char errmsg[STRERR_BUFSIZE];
> >> > struct bpf_link *link;
> >> > int prog_fd, link_fd;
> >> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
> >> > + .target_btf_id = target_btf_id);
> >> >
> >> > prog_fd = bpf_program__fd(prog);
> >> > if (prog_fd < 0) {
> >> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> >> > return ERR_PTR(-ENOMEM);
> >> > link->detach = &bpf_link__detach_fd;
> >> >
> >> > - attach_type = bpf_program__get_expected_attach_type(prog);
> >> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
> >> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
> >> > + attach_type = BPF_TRACE_FREPLACE;
> >>
> >> doing this unconditionally will break an old-style freplace without
> >> target_fd/btf_id on older kernels. Safe and simple way would be to
> >> continue using raw_tracepoint_open when there is no target_fd/btf_id,
> >> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do
> >> feature detection, but it's still would be nice to handle old-style
> >> attach through raw_tracepoint_open for bpf_program__attach_freplace.
> >>
> >> so I suggest leaving bpf_program__attach_fd() as is and to create a
> >> custom bpf_program__attach_freplace() implementation.
>
> Sure, I'll take another pass at this. Not sure how useful feature
> detection in libbpf is; if the caller passes a target, libbpf can't
> really do much if the kernel doesn't support it...
I was thinking about bpf_program__attach_freplace(prog, 0, NULL) doing
bpf_raw_tracepoint_open(). It would be nice to support this, for API
uniformity, no?
>
> >> > + else
> >> > + attach_type = bpf_program__get_expected_attach_type(prog);
> >> > +
> >> > + link_fd = bpf_link_create(prog_fd, target_fd, attach_type, &opts);
> >> > if (link_fd < 0) {
> >> > link_fd = -errno;
> >> > free(link);
> >> > @@ -9357,19 +9363,25 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
> >> > struct bpf_link *
> >> > bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd)
> >> > {
> >> > - return bpf_program__attach_fd(prog, cgroup_fd, "cgroup");
> >> > + return bpf_program__attach_fd(prog, cgroup_fd, 0, "cgroup");
> >> > }
> >> >
> >> > struct bpf_link *
> >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
> >> > {
> >> > - return bpf_program__attach_fd(prog, netns_fd, "netns");
> >> > + return bpf_program__attach_fd(prog, netns_fd, 0, "netns");
> >> > }
> >> >
> >> > struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
> >> > {
> >> > /* target_fd/target_ifindex use the same field in LINK_CREATE */
> >> > - return bpf_program__attach_fd(prog, ifindex, "xdp");
> >> > + return bpf_program__attach_fd(prog, ifindex, 0, "xdp");
> >> > +}
> >> > +
> >> > +struct bpf_link *bpf_program__attach_freplace(struct bpf_program *prog,
> >> > + int target_fd, int target_btf_id)
> >> > +{
> >> > + return bpf_program__attach_fd(prog, target_fd, target_btf_id, "freplace");
> >> > }
> >> >
> >> > struct bpf_link *
> >> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> >> > index a750f67a23f6..ce5add9b9203 100644
> >> > --- a/tools/lib/bpf/libbpf.h
> >> > +++ b/tools/lib/bpf/libbpf.h
> >> > @@ -261,6 +261,9 @@ LIBBPF_API struct bpf_link *
> >> > bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
> >> > LIBBPF_API struct bpf_link *
> >> > bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
> >> > +LIBBPF_API struct bpf_link *
> >> > +bpf_program__attach_freplace(struct bpf_program *prog,
> >> > + int target_fd, int target_btf_id);
> >>
> >> maybe a const char * function name instead of target_btf_id would be a
> >> nicer API? Users won't have to deal with fetching target prog's BTF,
> >> searching it, etc. That's all pretty straightforward for libbpf to do,
> >> leaving users with more natural and simpler API.
> >>
> >
> > bpf_program__set_attach_target() uses string name for target
> > functions, we should definitely be consistent here
>
> All right, fair enough :)
>
> -Toke
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create
2020-09-16 21:24 ` Andrii Nakryiko
@ 2020-09-16 21:41 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-16 21:41 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Wed, Sep 16, 2020 at 2:21 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Wed, Sep 16, 2020 at 1:37 PM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> >>
>> >> 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>
>> >> >
>> >> > This adds support for supplying a target btf ID for the bpf_link_create()
>> >> > operation, and adds a new bpf_program__attach_freplace() high-level API for
>> >> > attaching freplace functions with a target.
>> >> >
>> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> > ---
>> >> > tools/lib/bpf/bpf.c | 1 +
>> >> > tools/lib/bpf/bpf.h | 3 ++-
>> >> > tools/lib/bpf/libbpf.c | 24 ++++++++++++++++++------
>> >> > tools/lib/bpf/libbpf.h | 3 +++
>> >> > tools/lib/bpf/libbpf.map | 1 +
>> >> > 5 files changed, 25 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> >> > index 82b983ff6569..e928456c0dd6 100644
>> >> > --- a/tools/lib/bpf/bpf.c
>> >> > +++ b/tools/lib/bpf/bpf.c
>> >> > @@ -599,6 +599,7 @@ int bpf_link_create(int prog_fd, int target_fd,
>> >> > attr.link_create.iter_info =
>> >> > ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0));
>> >> > attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0);
>> >> > + attr.link_create.target_btf_id = OPTS_GET(opts, target_btf_id, 0);
>> >> >
>> >> > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
>> >> > }
>> >> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> >> > index 015d13f25fcc..f8dbf666b62b 100644
>> >> > --- a/tools/lib/bpf/bpf.h
>> >> > +++ b/tools/lib/bpf/bpf.h
>> >> > @@ -174,8 +174,9 @@ struct bpf_link_create_opts {
>> >> > __u32 flags;
>> >> > union bpf_iter_link_info *iter_info;
>> >> > __u32 iter_info_len;
>> >> > + __u32 target_btf_id;
>> >> > };
>> >> > -#define bpf_link_create_opts__last_field iter_info_len
>> >> > +#define bpf_link_create_opts__last_field target_btf_id
>> >> >
>> >> > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
>> >> > enum bpf_attach_type attach_type,
>> >> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> >> > index 550950eb1860..165131c73f40 100644
>> >> > --- a/tools/lib/bpf/libbpf.c
>> >> > +++ b/tools/lib/bpf/libbpf.c
>> >> > @@ -9322,12 +9322,14 @@ static struct bpf_link *attach_iter(const struct bpf_sec_def *sec,
>> >> >
>> >> > static struct bpf_link *
>> >> > bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
>> >> > - const char *target_name)
>> >> > + int target_btf_id, const char *target_name)
>> >> > {
>> >> > enum bpf_attach_type attach_type;
>> >> > char errmsg[STRERR_BUFSIZE];
>> >> > struct bpf_link *link;
>> >> > int prog_fd, link_fd;
>> >> > + DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
>> >> > + .target_btf_id = target_btf_id);
>> >> >
>> >> > prog_fd = bpf_program__fd(prog);
>> >> > if (prog_fd < 0) {
>> >> > @@ -9340,8 +9342,12 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,
>> >> > return ERR_PTR(-ENOMEM);
>> >> > link->detach = &bpf_link__detach_fd;
>> >> >
>> >> > - attach_type = bpf_program__get_expected_attach_type(prog);
>> >> > - link_fd = bpf_link_create(prog_fd, target_fd, attach_type, NULL);
>> >> > + if (bpf_program__get_type(prog) == BPF_PROG_TYPE_EXT)
>> >> > + attach_type = BPF_TRACE_FREPLACE;
>> >>
>> >> doing this unconditionally will break an old-style freplace without
>> >> target_fd/btf_id on older kernels. Safe and simple way would be to
>> >> continue using raw_tracepoint_open when there is no target_fd/btf_id,
>> >> and use LINK_CREATE for newer stuff. Alternatively, you'd need to do
>> >> feature detection, but it's still would be nice to handle old-style
>> >> attach through raw_tracepoint_open for bpf_program__attach_freplace.
>> >>
>> >> so I suggest leaving bpf_program__attach_fd() as is and to create a
>> >> custom bpf_program__attach_freplace() implementation.
>>
>> Sure, I'll take another pass at this. Not sure how useful feature
>> detection in libbpf is; if the caller passes a target, libbpf can't
>> really do much if the kernel doesn't support it...
>
> I was thinking about bpf_program__attach_freplace(prog, 0, NULL) doing
> bpf_raw_tracepoint_open(). It would be nice to support this, for API
> uniformity, no?
Yeah, sure, that we can do :)
-Toke
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next v5 7/8] selftests: add test for multiple attachments of freplace program
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
` (5 preceding siblings ...)
2020-09-15 11:41 ` [PATCH bpf-next v5 6/8] libbpf: add support for freplace attachment in bpf_link_create Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-15 11:41 ` [PATCH bpf-next v5 8/8] selftests/bpf: Adding test for arg dereference in extension trace Toke Høiland-Jørgensen
7 siblings, 0 replies; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Toke Høiland-Jørgensen <toke@redhat.com>
This adds a selftest for attaching an freplace program to multiple targets
simultaneously.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
.../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 171 ++++++++++++++++----
.../selftests/bpf/progs/freplace_get_constant.c | 15 ++
2 files changed, 154 insertions(+), 32 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index eda682727787..3621d66e47dd 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -2,36 +2,79 @@
/* Copyright (c) 2019 Facebook */
#include <test_progs.h>
#include <network_helpers.h>
+#include <bpf/btf.h>
+
+typedef int (*test_cb)(struct bpf_object *obj);
+
+static int check_data_map(struct bpf_object *obj, int prog_cnt, bool reset)
+{
+ struct bpf_map *data_map = NULL, *map;
+ __u64 *result = NULL;
+ const int zero = 0;
+ __u32 duration = 0;
+ int ret = -1, i;
+
+ result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
+ if (CHECK(!result, "alloc_memory", "failed to alloc memory"))
+ return -ENOMEM;
+
+ bpf_object__for_each_map(map, obj)
+ if (bpf_map__is_internal(map)) {
+ data_map = map;
+ break;
+ }
+ if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+ goto out;
+
+ ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
+ if (CHECK(ret, "get_result",
+ "failed to get output data: %d\n", ret))
+ goto out;
+
+ for (i = 0; i < prog_cnt; i++) {
+ if (CHECK(result[i] != 1, "result",
+ "fexit_bpf2bpf result[%d] failed err %llu\n",
+ i, result[i]))
+ goto out;
+ result[i] = 0;
+ }
+ if (reset) {
+ ret = bpf_map_update_elem(bpf_map__fd(data_map), &zero, result, 0);
+ if (CHECK(ret, "reset_result", "failed to reset result\n"))
+ goto out;
+ }
+
+ ret = 0;
+out:
+ free(result);
+ return ret;
+}
static void test_fexit_bpf2bpf_common(const char *obj_file,
const char *target_obj_file,
int prog_cnt,
const char **prog_name,
- bool run_prog)
+ bool run_prog,
+ test_cb cb)
{
- struct bpf_object *obj = NULL, *pkt_obj;
- int err, pkt_fd, i;
- struct bpf_link **link = NULL;
+ struct bpf_object *obj = NULL, *tgt_obj;
struct bpf_program **prog = NULL;
+ struct bpf_link **link = NULL;
__u32 duration = 0, retval;
- struct bpf_map *data_map;
- const int zero = 0;
- __u64 *result = NULL;
+ int err, tgt_fd, i;
err = bpf_prog_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
- &pkt_obj, &pkt_fd);
+ &tgt_obj, &tgt_fd);
if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
target_obj_file, err, errno))
return;
DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
- .attach_prog_fd = pkt_fd,
+ .attach_prog_fd = tgt_fd,
);
link = calloc(sizeof(struct bpf_link *), prog_cnt);
prog = calloc(sizeof(struct bpf_program *), prog_cnt);
- result = malloc((prog_cnt + 32 /* spare */) * sizeof(__u64));
- if (CHECK(!link || !prog || !result, "alloc_memory",
- "failed to alloc memory"))
+ if (CHECK(!link || !prog, "alloc_memory", "failed to alloc memory"))
goto close_prog;
obj = bpf_object__open_file(obj_file, &opts);
@@ -53,39 +96,33 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
goto close_prog;
}
- if (!run_prog)
- goto close_prog;
+ if (cb) {
+ err = cb(obj);
+ if (err)
+ goto close_prog;
+ }
- data_map = bpf_object__find_map_by_name(obj, "fexit_bp.bss");
- if (CHECK(!data_map, "find_data_map", "data map not found\n"))
+ if (!run_prog)
goto close_prog;
- err = bpf_prog_test_run(pkt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+ err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6),
NULL, NULL, &retval, &duration);
CHECK(err || retval, "ipv6",
"err %d errno %d retval %d duration %d\n",
err, errno, retval, duration);
- err = bpf_map_lookup_elem(bpf_map__fd(data_map), &zero, result);
- if (CHECK(err, "get_result",
- "failed to get output data: %d\n", err))
+ if (check_data_map(obj, prog_cnt, false))
goto close_prog;
- for (i = 0; i < prog_cnt; i++)
- if (CHECK(result[i] != 1, "result", "fexit_bpf2bpf failed err %llu\n",
- result[i]))
- goto close_prog;
-
close_prog:
for (i = 0; i < prog_cnt; i++)
if (!IS_ERR_OR_NULL(link[i]))
bpf_link__destroy(link[i]);
if (!IS_ERR_OR_NULL(obj))
bpf_object__close(obj);
- bpf_object__close(pkt_obj);
+ bpf_object__close(tgt_obj);
free(link);
free(prog);
- free(result);
}
static void test_target_no_callees(void)
@@ -96,7 +133,7 @@ static void test_target_no_callees(void)
test_fexit_bpf2bpf_common("./fexit_bpf2bpf_simple.o",
"./test_pkt_md_access.o",
ARRAY_SIZE(prog_name),
- prog_name, true);
+ prog_name, true, NULL);
}
static void test_target_yes_callees(void)
@@ -110,7 +147,7 @@ static void test_target_yes_callees(void)
test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
"./test_pkt_access.o",
ARRAY_SIZE(prog_name),
- prog_name, true);
+ prog_name, true, NULL);
}
static void test_func_replace(void)
@@ -128,7 +165,7 @@ static void test_func_replace(void)
test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
"./test_pkt_access.o",
ARRAY_SIZE(prog_name),
- prog_name, true);
+ prog_name, true, NULL);
}
static void test_func_replace_verify(void)
@@ -139,7 +176,75 @@ static void test_func_replace_verify(void)
test_fexit_bpf2bpf_common("./freplace_connect4.o",
"./connect4_prog.o",
ARRAY_SIZE(prog_name),
- prog_name, false);
+ prog_name, false, NULL);
+}
+
+static int test_second_attach(struct bpf_object *obj)
+{
+ const char *prog_name = "freplace/get_constant";
+ const char *tgt_name = prog_name + 9; /* cut off freplace/ */
+ const char *tgt_obj_file = "./test_pkt_access.o";
+ int err = 0, tgt_fd, tgt_btf_id, link_fd = -1;
+ struct bpf_program *prog = NULL;
+ struct bpf_object *tgt_obj;
+ __u32 duration = 0, retval;
+ struct btf *btf;
+
+ prog = bpf_object__find_program_by_title(obj, prog_name);
+ if (CHECK(!prog, "find_prog", "prog %s not found\n", prog_name))
+ return -ENOENT;
+
+ err = bpf_prog_load(tgt_obj_file, BPF_PROG_TYPE_UNSPEC,
+ &tgt_obj, &tgt_fd);
+ if (CHECK(err, "second_prog_load", "file %s err %d errno %d\n",
+ tgt_obj_file, err, errno))
+ return err;
+
+ btf = bpf_object__btf(tgt_obj);
+ tgt_btf_id = btf__find_by_name_kind(btf, tgt_name, BTF_KIND_FUNC);
+ if (CHECK(tgt_btf_id < 0, "find_btf", "no BTF ID found for func %s\n", prog_name)) {
+ err = -ENOENT;
+ goto out;
+ }
+
+ DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts,
+ .target_btf_id = tgt_btf_id,
+ );
+ link_fd = bpf_link_create(bpf_program__fd(prog), tgt_fd,
+ BPF_TRACE_FREPLACE, &opts);
+ if (CHECK(link_fd < 0, "second_link", "err %d errno %d",
+ link_fd, errno)) {
+ err = link_fd;
+ goto out;
+ }
+
+ err = bpf_prog_test_run(tgt_fd, 1, &pkt_v6, sizeof(pkt_v6),
+ NULL, NULL, &retval, &duration);
+ if (CHECK(err || retval, "ipv6",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration))
+ goto out;
+
+ err = check_data_map(obj, 1, true);
+ if (err)
+ goto out;
+
+out:
+ if (link_fd >= 0)
+ close(link_fd);
+ bpf_object__close(tgt_obj);
+ return err;
+}
+
+static void test_func_replace_multi(void)
+{
+ const char *prog_name[] = {
+ "freplace/get_constant",
+ };
+ test_fexit_bpf2bpf_common("./freplace_get_constant.o",
+ "./test_pkt_access.o",
+ ARRAY_SIZE(prog_name),
+ prog_name, true, test_second_attach);
}
static void test_func_sockmap_update(void)
@@ -150,7 +255,7 @@ static void test_func_sockmap_update(void)
test_fexit_bpf2bpf_common("./freplace_cls_redirect.o",
"./test_cls_redirect.o",
ARRAY_SIZE(prog_name),
- prog_name, false);
+ prog_name, false, NULL);
}
static void test_obj_load_failure_common(const char *obj_file,
@@ -222,4 +327,6 @@ void test_fexit_bpf2bpf(void)
test_func_replace_return_code();
if (test__start_subtest("func_map_prog_compatibility"))
test_func_map_prog_compatibility();
+ if (test__start_subtest("func_replace_multi"))
+ test_func_replace_multi();
}
diff --git a/tools/testing/selftests/bpf/progs/freplace_get_constant.c b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
new file mode 100644
index 000000000000..8f0ecf94e533
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_get_constant.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+volatile __u64 test_get_constant = 0;
+SEC("freplace/get_constant")
+int new_get_constant(long val)
+{
+ if (val != 123)
+ return 0;
+ test_get_constant = 1;
+ return test_get_constant; /* original get_constant() returns val - 122 */
+}
+char _license[] SEC("license") = "GPL";
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH bpf-next v5 8/8] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-15 11:40 [PATCH bpf-next v5 0/8] bpf: Support multi-attach for freplace programs Toke Høiland-Jørgensen
` (6 preceding siblings ...)
2020-09-15 11:41 ` [PATCH bpf-next v5 7/8] selftests: add test for multiple attachments of freplace program Toke Høiland-Jørgensen
@ 2020-09-15 11:41 ` Toke Høiland-Jørgensen
2020-09-16 20:44 ` Andrii Nakryiko
7 siblings, 1 reply; 33+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-09-15 11:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, Jiri Olsa, Eelco Chaudron,
KP Singh, netdev, bpf
From: Jiri Olsa <jolsa@kernel.org>
Adding test that setup following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
The test verifies that the tracing program can
dereference skb argument properly.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
tools/testing/selftests/bpf/prog_tests/trace_ext.c | 113 ++++++++++++++++++++
tools/testing/selftests/bpf/progs/test_trace_ext.c | 18 +++
.../selftests/bpf/progs/test_trace_ext_tracing.c | 25 ++++
3 files changed, 156 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_ext.c b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
new file mode 100644
index 000000000000..49b554f560a4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_ext.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <network_helpers.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#include "test_pkt_md_access.skel.h"
+#include "test_trace_ext.skel.h"
+#include "test_trace_ext_tracing.skel.h"
+
+static __u32 duration;
+
+void test_trace_ext(void)
+{
+ struct test_pkt_md_access *skel_pkt = NULL;
+ struct test_trace_ext_tracing *skel_trace = NULL;
+ struct test_trace_ext_tracing__bss *bss_trace;
+ struct test_trace_ext *skel_ext = NULL;
+ struct test_trace_ext__bss *bss_ext;
+ int err, pkt_fd, ext_fd;
+ struct bpf_program *prog;
+ char buf[100];
+ __u32 retval;
+ __u64 len;
+
+ /* open/load/attach test_pkt_md_access */
+ skel_pkt = test_pkt_md_access__open_and_load();
+ if (CHECK(!skel_pkt, "setup", "classifier/test_pkt_md_access open failed\n"))
+ goto cleanup;
+
+ err = test_pkt_md_access__attach(skel_pkt);
+ if (CHECK(err, "setup", "classifier/test_pkt_md_access attach failed: %d\n", err))
+ goto cleanup;
+
+ prog = skel_pkt->progs.test_pkt_md_access;
+ pkt_fd = bpf_program__fd(prog);
+
+ /* open extension */
+ skel_ext = test_trace_ext__open();
+ if (CHECK(!skel_ext, "setup", "freplace/test_pkt_md_access open failed\n"))
+ goto cleanup;
+
+ /* set extension's attach target - test_pkt_md_access */
+ prog = skel_ext->progs.test_pkt_md_access_new;
+ bpf_program__set_attach_target(prog, pkt_fd, "test_pkt_md_access");
+
+ /* load/attach extension */
+ err = test_trace_ext__load(skel_ext);
+ if (CHECK(err, "setup", "freplace/test_pkt_md_access load failed\n")) {
+ libbpf_strerror(err, buf, sizeof(buf));
+ fprintf(stderr, "%s\n", buf);
+ goto cleanup;
+ }
+
+ err = test_trace_ext__attach(skel_ext);
+ if (CHECK(err, "setup", "freplace/test_pkt_md_access attach failed: %d\n", err))
+ goto cleanup;
+
+ prog = skel_ext->progs.test_pkt_md_access_new;
+ ext_fd = bpf_program__fd(prog);
+
+ /* open tracing */
+ skel_trace = test_trace_ext_tracing__open();
+ if (CHECK(!skel_trace, "setup", "tracing/test_pkt_md_access_new open failed\n"))
+ goto cleanup;
+
+ /* set tracing's attach target - fentry */
+ prog = skel_trace->progs.fentry;
+ bpf_program__set_attach_target(prog, ext_fd, "test_pkt_md_access_new");
+
+ /* set tracing's attach target - fexit */
+ prog = skel_trace->progs.fexit;
+ bpf_program__set_attach_target(prog, ext_fd, "test_pkt_md_access_new");
+
+ /* load/attach tracing */
+ err = test_trace_ext_tracing__load(skel_trace);
+ if (CHECK(err, "setup", "tracing/test_pkt_md_access_new load failed\n")) {
+ libbpf_strerror(err, buf, sizeof(buf));
+ fprintf(stderr, "%s\n", buf);
+ goto cleanup;
+ }
+
+ err = test_trace_ext_tracing__attach(skel_trace);
+ if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
+ goto cleanup;
+
+ /* trigger the test */
+ err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4),
+ NULL, NULL, &retval, &duration);
+ CHECK(err || retval, "",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration);
+
+ bss_ext = skel_ext->bss;
+ bss_trace = skel_trace->bss;
+
+ len = bss_ext->ext_called;
+
+ CHECK(bss_ext->ext_called == 0,
+ "check", "failed to trigger freplace/test_pkt_md_access\n");
+ CHECK(bss_trace->fentry_called != len,
+ "check", "failed to trigger fentry/test_pkt_md_access_new\n");
+ CHECK(bss_trace->fexit_called != len,
+ "check", "failed to trigger fexit/test_pkt_md_access_new\n");
+
+cleanup:
+ test_trace_ext_tracing__destroy(skel_trace);
+ test_trace_ext__destroy(skel_ext);
+ test_pkt_md_access__destroy(skel_pkt);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
new file mode 100644
index 000000000000..d19a634d0e78
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Facebook
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 ext_called = 0;
+
+SEC("freplace/test_pkt_md_access")
+int test_pkt_md_access_new(struct __sk_buff *skb)
+{
+ ext_called = skb->len;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
new file mode 100644
index 000000000000..52f3baf98f20
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 fentry_called = 0;
+
+SEC("fentry/test_pkt_md_access_new")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+ fentry_called = skb->len;
+ return 0;
+}
+
+__u64 fexit_called = 0;
+
+SEC("fexit/test_pkt_md_access_new")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+ fexit_called = skb->len;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next v5 8/8] selftests/bpf: Adding test for arg dereference in extension trace
2020-09-15 11:41 ` [PATCH bpf-next v5 8/8] selftests/bpf: Adding test for arg dereference in extension trace Toke Høiland-Jørgensen
@ 2020-09-16 20:44 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2020-09-16 20:44 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, Jiri Olsa,
Eelco Chaudron, KP Singh, Networking, bpf
On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Jiri Olsa <jolsa@kernel.org>
>
> Adding test that setup following program:
>
> SEC("classifier/test_pkt_md_access")
> int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
> SEC("freplace/test_pkt_md_access")
> int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
> SEC("fentry/test_pkt_md_access_new")
> int BPF_PROG(fentry, struct sk_buff *skb)
>
> The test verifies that the tracing program can
> dereference skb argument properly.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
Looks good, with a minor CHECK() complaint below.
Acked-by: Andrii Nakryiko <andriin@fb.com>
> tools/testing/selftests/bpf/prog_tests/trace_ext.c | 113 ++++++++++++++++++++
> tools/testing/selftests/bpf/progs/test_trace_ext.c | 18 +++
> .../selftests/bpf/progs/test_trace_ext_tracing.c | 25 ++++
> 3 files changed, 156 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
>
[...]
> + err = test_trace_ext_tracing__attach(skel_trace);
> + if (CHECK(err, "setup", "tracing/test_pkt_md_access_new attach failed: %d\n", err))
> + goto cleanup;
> +
> + /* trigger the test */
> + err = bpf_prog_test_run(pkt_fd, 1, &pkt_v4, sizeof(pkt_v4),
> + NULL, NULL, &retval, &duration);
> + CHECK(err || retval, "",
please don't use empty string here
> + "err %d errno %d retval %d duration %d\n",
> + err, errno, retval, duration);
who and why cares about duration here?..
> +
> + bss_ext = skel_ext->bss;
> + bss_trace = skel_trace->bss;
> +
> + len = bss_ext->ext_called;
> +
> + CHECK(bss_ext->ext_called == 0,
> + "check", "failed to trigger freplace/test_pkt_md_access\n");
> + CHECK(bss_trace->fentry_called != len,
> + "check", "failed to trigger fentry/test_pkt_md_access_new\n");
> + CHECK(bss_trace->fexit_called != len,
> + "check", "failed to trigger fexit/test_pkt_md_access_new\n");
> +
> +cleanup:
> + test_trace_ext_tracing__destroy(skel_trace);
> + test_trace_ext__destroy(skel_ext);
> + test_pkt_md_access__destroy(skel_pkt);
> +}
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread