On Mon, Mar 21, 2022 at 6:15 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Mar 21, 2022 at 4:24 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > I remember I brought this up earlier, but I forgot the outcome. What
> > if don't touch BPF_RAW_TRACEPOINT_OPEN and instead allow to create all
> > the same links through more universal BPF_LINK_CREATE command. And
> > only there we add bpf_cookie? There are few advantages:
> >
> > 1. We can separate raw_tracepoint and trampoline-based programs more
> > cleanly in UAPI (it will be two separate structs: link_create.raw_tp
> > with raw tracepoint name vs link_create.trampoline, or whatever the
> > name, with cookie and stuff). Remember that raw_tp won't support
> > bpf_cookie for now, so it would be another advantage not to promise
> > cookie in UAPI.
>
> What would it look like?
> Technically link_create has prog_fd and perf_event.bpf_cookie
> already.
>
> case BPF_PROG_TYPE_TRACING:
> ret = tracing_bpf_link_attach(attr, uattr, prog);
> would just gain a few more checks for prog->expected_attach_type ?
>
> Then link_create cmd will be equivalent to raw_tp_open.
> With and without bpf_cookie.
> ?
Yes, except I'd leave perf_event for perf_event-based attachments
(kprobe, uprobe, tracepoint) and would define a separate substruct for
trampoline-based programs. Something like this (I only compile-tested
it, of course). I've also simplified prog_type/expected_attach_type
logic a bit because it felt like a total maze to me and I was getting
lost all the time. Gmail will probably corrupt all the whitespaces,
sorry about that in advance.
Seems like we could already attach BPF_PROG_TYPE_EXT both through
RAW_TRACEPOINT_OPEN and LINK_CREATE, I didn't realize that. The
"patch" below leaves raw_tp handling
(BPF_PROG_TYPE_TRACING+BPF_TRACE_RAW_TP, BPF_PROG_TYPE_RAW_TRACEPOINT,
and BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) in RAW_TRACEPOINT_OPEN. If
we want to completely unify all the bpf_link creations under
LINK_CREATE, see extra small "patch" at the very bottom.
P.S. While looking at this, I also realized that we should probably
enforce zeros after all those kprobe_multi, trampoline, perf_event,
etc sections (depending on specific program type). We missed that
initially and don't enforce it right now (I don't think anyone sane
would rely on this, so probably best to fix this soon-ish).
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d14b10b85e51..5692d4381e3b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1489,6 +1489,9 @@ union bpf_attr {
__aligned_u64 addrs;
__aligned_u64 cookies;
} kprobe_multi;
+ struct {
+ __u64 cookie;
+ } trampoline;
};
} link_create;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cdaa1152436a..819f30952cc4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2704,7 +2704,7 @@ static const struct bpf_link_ops bpf_tracing_link_lops = {
static int bpf_tracing_prog_attach(struct bpf_prog *prog,
int tgt_prog_fd,
- u32 btf_id)
+ u32 btf_id, u64 cookie)
{
struct bpf_link_primer link_primer;
struct bpf_prog *tgt_prog = NULL;
@@ -2840,7 +2840,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
if (err)
goto out_unlock;
- err = bpf_trampoline_link_prog(prog, tr);
+ err = bpf_trampoline_link_prog(prog, tr /* , cookie */);
if (err) {
bpf_link_cleanup(&link_primer);
link = NULL;
@@ -3065,7 +3065,7 @@ static int bpf_raw_tracepoint_open(const union
bpf_attr *attr)
tp_name = prog->aux->attach_func_name;
break;
}
- err = bpf_tracing_prog_attach(prog, 0, 0);
+ err = bpf_tracing_prog_attach(prog, 0, 0, 0);
if (err >= 0)
return err;
goto out_put_prog;
@@ -3189,7 +3189,12 @@ attach_type_to_prog_type(enum bpf_attach_type
attach_type)
case BPF_CGROUP_SETSOCKOPT:
return BPF_PROG_TYPE_CGROUP_SOCKOPT;
case BPF_TRACE_ITER:
+ case BPF_TRACE_FENTRY:
+ case BPF_TRACE_FEXIT:
+ case BPF_MODIFY_RETURN:
return BPF_PROG_TYPE_TRACING;
+ case BPF_LSM_MAC:
+ return BPF_PROG_TYPE_LSM;
case BPF_SK_LOOKUP:
return BPF_PROG_TYPE_SK_LOOKUP;
case BPF_XDP:
@@ -4246,21 +4251,6 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
return err;
}
-static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
- struct bpf_prog *prog)
-{
- if (attr->link_create.attach_type != prog->expected_attach_type)
- return -EINVAL;
-
- if (prog->expected_attach_type == BPF_TRACE_ITER)
- return bpf_iter_link_attach(attr, uattr, prog);
- else if (prog->type == BPF_PROG_TYPE_EXT)
- 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.kprobe_multi.cookies
static int link_create(union bpf_attr *attr, bpfptr_t uattr)
{
@@ -4282,15 +4272,13 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
switch (prog->type) {
case BPF_PROG_TYPE_EXT:
- ret = tracing_bpf_link_attach(attr, uattr, prog);
- goto out;
+ break;
case BPF_PROG_TYPE_PERF_EVENT:
case BPF_PROG_TYPE_TRACEPOINT:
if (attr->link_create.attach_type != BPF_PERF_EVENT) {
ret = -EINVAL;
goto out;
}
- ptype = prog->type;
break;
case BPF_PROG_TYPE_KPROBE:
if (attr->link_create.attach_type != BPF_PERF_EVENT &&
@@ -4298,7 +4286,6 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
ret = -EINVAL;
goto out;
}
- ptype = prog->type;
break;
default:
ptype = attach_type_to_prog_type(attr->link_create.attach_type);
@@ -4309,7 +4296,7 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
break;
}
- switch (ptype) {
+ switch (prog->type) {
case BPF_PROG_TYPE_CGROUP_SKB:
case BPF_PROG_TYPE_CGROUP_SOCK:
case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
@@ -4319,8 +4306,25 @@ static int link_create(union bpf_attr *attr,
bpfptr_t uattr)
case BPF_PROG_TYPE_CGROUP_SOCKOPT:
ret = cgroup_bpf_link_attach(attr, prog);
break;
+ case BPF_PROG_TYPE_EXT:
+ ret = bpf_tracing_prog_attach(prog,
+ attr->link_create.target_fd,
+ attr->link_create.target_btf_id,
+ attr->link_create.trampoline.cookie);
+ break;
+ case BPF_PROG_TYPE_LSM:
case BPF_PROG_TYPE_TRACING:
- ret = tracing_bpf_link_attach(attr, uattr, prog);
+ if (attr->link_create.attach_type != prog->expected_attach_type) {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (prog->expected_attach_type == BPF_TRACE_ITER)
+ ret = bpf_iter_link_attach(attr, uattr, prog);
+ else
+ ret = bpf_tracing_prog_attach(prog,
+ attr->link_create.target_fd,
+ attr->link_create.target_btf_id,
+ attr->link_create.trampoline.cookie);
break;
case BPF_PROG_TYPE_FLOW_DISSECTOR:
case BPF_PROG_TYPE_SK_LOOKUP:
And then for moving raw_tp-related attachments into LINK_CREATE, I'd
add this to UAPI and wire everything in kernel/bpf/syscall.c
accordingly:
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 5692d4381e3b..a85403f89569 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1492,6 +1492,9 @@ union bpf_attr {
struct {
__u64 cookie;
} trampoline;
+ struct {
+ __aligned_u64 name;
+ } raw_tp;
};
} link_create;
