* [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id
@ 2022-07-30 0:08 Stanislav Fomichev
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2022-07-30 0:08 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
When attaching to program, the program itself might not be attached
to anything (and, hence, might not have attach_btf), so we can't
unconditionally use 'prog->aux->dst_prog->aux->attach_btf'.
Instead, use bpf_prog_get_target_btf to pick proper target btf:
* when attached to dst_prog, use dst_prog->aux->btf
* when attached to kernel btf, use prog->aux->attach_btf
Fixes: b79c9fc9551b ("bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/syscall.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 83c7136c5788..7dc3f8003631 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3886,6 +3886,7 @@ static int bpf_prog_get_info_by_fd(struct file *file,
union bpf_attr __user *uattr)
{
struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
+ struct btf *attach_btf = bpf_prog_get_target_btf(prog);
struct bpf_prog_info info;
u32 info_len = attr->info.info_len;
struct bpf_prog_kstats stats;
@@ -4088,10 +4089,8 @@ static int bpf_prog_get_info_by_fd(struct file *file,
if (prog->aux->btf)
info.btf_id = btf_obj_id(prog->aux->btf);
info.attach_btf_id = prog->aux->attach_btf_id;
- if (prog->aux->attach_btf)
- info.attach_btf_obj_id = btf_obj_id(prog->aux->attach_btf);
- else if (prog->aux->dst_prog)
- info.attach_btf_obj_id = btf_obj_id(prog->aux->dst_prog->aux->attach_btf);
+ if (attach_btf)
+ info.attach_btf_obj_id = btf_obj_id(attach_btf);
ulen = info.nr_func_info;
info.nr_func_info = prog->aux->func_info_cnt;
--
2.37.1.455.g008518b4e5-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-07-30 0:08 [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Stanislav Fomichev
@ 2022-07-30 0:08 ` Stanislav Fomichev
2022-07-30 0:56 ` Hao Luo
2022-08-01 17:45 ` Martin KaFai Lau
2022-07-30 0:24 ` [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Hao Luo
2022-08-01 16:57 ` Martin KaFai Lau
2 siblings, 2 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2022-07-30 0:08 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Apparently, no existing selftest covers it. Add a new one where
we load cgroup/bind4 program and attach fentry to it.
Calling bpf_obj_get_info_by_fd on the fentry program
should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
.../selftests/bpf/progs/attach_to_bpf.c | 12 ++
2 files changed, 121 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
new file mode 100644
index 000000000000..fcf726c5ff0f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include <bpf/btf.h>
+#include <test_progs.h>
+#include <network_helpers.h>
+#include "attach_to_bpf.skel.h"
+
+char bpf_log_buf[BPF_LOG_BUF_SIZE];
+
+static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ struct btf *btf;
+ int err;
+
+ err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
+ if (err)
+ return err;
+
+ if (!info.btf_id)
+ return -EINVAL;
+
+ btf = btf__load_from_kernel_by_id(info.btf_id);
+ err = libbpf_get_error(btf);
+ if (err)
+ return err;
+
+ err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
+ btf__free(btf);
+ if (err <= 0)
+ return err;
+
+ return err;
+}
+
+int load_fentry(int attach_prog_fd, int attach_btf_id)
+{
+ LIBBPF_OPTS(bpf_prog_load_opts, opts,
+ .expected_attach_type = BPF_TRACE_FENTRY,
+ .attach_prog_fd = attach_prog_fd,
+ .attach_btf_id = attach_btf_id,
+ .log_buf = bpf_log_buf,
+ .log_size = sizeof(bpf_log_buf),
+ );
+ struct bpf_insn insns[] = {
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ };
+ int ret;
+
+ ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
+ "bind4_fentry",
+ "GPL",
+ insns,
+ ARRAY_SIZE(insns),
+ &opts);
+ if (ret)
+ printf("verifier log: %s\n", bpf_log_buf);
+ return ret;
+}
+
+void test_attach_to_bpf(void)
+{
+ struct attach_to_bpf *skel = NULL;
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ int cgroup_fd = -1;
+ int fentry_fd = -1;
+ int btf_id;
+
+ cgroup_fd = test__join_cgroup("/attach_to_bpf");
+ if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
+ return;
+
+ skel = attach_to_bpf__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel"))
+ goto cleanup;
+
+ skel->links.bind4 = bpf_program__attach_cgroup(skel->progs.bind4, cgroup_fd);
+ if (!ASSERT_OK_PTR(skel, "bpf_program__attach_cgroup"))
+ goto cleanup;
+
+ btf_id = find_prog_btf_id("bind4", bpf_program__fd(skel->progs.bind4));
+ if (!ASSERT_GE(btf_id, 0, "find_prog_btf_id"))
+ goto cleanup;
+
+ fentry_fd = load_fentry(bpf_program__fd(skel->progs.bind4), btf_id);
+ if (!ASSERT_GE(fentry_fd, 0, "load_fentry"))
+ goto cleanup;
+
+ /* Make sure bpf_obj_get_info_by_fd works correctly when attaching
+ * to another BPF program.
+ */
+
+ if (!ASSERT_OK(bpf_obj_get_info_by_fd(fentry_fd, &info, &info_len),
+ "bpf_obj_get_info_by_fd"))
+ goto cleanup;
+
+ ASSERT_EQ(info.btf_id, 0, "info.btf_id");
+ ASSERT_GT(info.attach_btf_id, 0, "info.attach_btf_id");
+ ASSERT_GT(info.attach_btf_obj_id, 0, "info.attach_btf_obj_id");
+
+cleanup:
+ close(cgroup_fd);
+ close(fentry_fd);
+ attach_to_bpf__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/attach_to_bpf.c b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
new file mode 100644
index 000000000000..3f111fe96f8f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("cgroup/bind4")
+int bind4(struct bpf_sock_addr *ctx)
+{
+ return 1;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.37.1.455.g008518b4e5-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id
2022-07-30 0:08 [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Stanislav Fomichev
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
@ 2022-07-30 0:24 ` Hao Luo
2022-08-01 16:57 ` Martin KaFai Lau
2 siblings, 0 replies; 9+ messages in thread
From: Hao Luo @ 2022-07-30 0:24 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, jolsa
On Fri, Jul 29, 2022 at 5:08 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> When attaching to program, the program itself might not be attached
> to anything (and, hence, might not have attach_btf), so we can't
> unconditionally use 'prog->aux->dst_prog->aux->attach_btf'.
> Instead, use bpf_prog_get_target_btf to pick proper target btf:
>
> * when attached to dst_prog, use dst_prog->aux->btf
> * when attached to kernel btf, use prog->aux->attach_btf
>
> Fixes: b79c9fc9551b ("bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Looks good to me.
Acked-by: Hao Luo <haoluo@google.com>
Hao
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
@ 2022-07-30 0:56 ` Hao Luo
2022-07-30 2:16 ` Stanislav Fomichev
2022-08-01 17:45 ` Martin KaFai Lau
1 sibling, 1 reply; 9+ messages in thread
From: Hao Luo @ 2022-07-30 0:56 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, jolsa
On Fri, Jul 29, 2022 at 5:08 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Apparently, no existing selftest covers it. Add a new one where
> we load cgroup/bind4 program and attach fentry to it.
> Calling bpf_obj_get_info_by_fd on the fentry program
> should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
Looks good to me overall, with a few nits.
> .../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
> .../selftests/bpf/progs/attach_to_bpf.c | 12 ++
> 2 files changed, 121 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> new file mode 100644
> index 000000000000..fcf726c5ff0f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include <bpf/btf.h>
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include "attach_to_bpf.skel.h"
> +
> +char bpf_log_buf[BPF_LOG_BUF_SIZE];
> +
> +static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> +{
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + struct btf *btf;
> + int err;
> +
> + err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
> + if (err)
> + return err;
> +
> + if (!info.btf_id)
> + return -EINVAL;
> +
> + btf = btf__load_from_kernel_by_id(info.btf_id);
> + err = libbpf_get_error(btf);
> + if (err)
> + return err;
> +
> + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
nit: Prefer use a separate variable like 'btf_id' instead of using 'err'.
> + btf__free(btf);
> + if (err <= 0)
> + return err;
> +
> + return err;
> +}
> +
> +int load_fentry(int attach_prog_fd, int attach_btf_id)
> +{
> + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> + .expected_attach_type = BPF_TRACE_FENTRY,
> + .attach_prog_fd = attach_prog_fd,
> + .attach_btf_id = attach_btf_id,
> + .log_buf = bpf_log_buf,
> + .log_size = sizeof(bpf_log_buf),
> + );
> + struct bpf_insn insns[] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + int ret;
> +
> + ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
> + "bind4_fentry",
> + "GPL",
> + insns,
> + ARRAY_SIZE(insns),
> + &opts);
> + if (ret)
> + printf("verifier log: %s\n", bpf_log_buf);
Do we need to print log? If print logging isn't necessary, we could
directly return bpf_prog_load(). Seems simpler.
> + return ret;
> +}
> +
> +void test_attach_to_bpf(void)
> +{
> + struct attach_to_bpf *skel = NULL;
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + int cgroup_fd = -1;
> + int fentry_fd = -1;
> + int btf_id;
> +
> + cgroup_fd = test__join_cgroup("/attach_to_bpf");
> + if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
> + return;
> +
> + skel = attach_to_bpf__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel"))
> + goto cleanup;
> +
> + skel->links.bind4 = bpf_program__attach_cgroup(skel->progs.bind4, cgroup_fd);
> + if (!ASSERT_OK_PTR(skel, "bpf_program__attach_cgroup"))
> + goto cleanup;
> +
> + btf_id = find_prog_btf_id("bind4", bpf_program__fd(skel->progs.bind4));
> + if (!ASSERT_GE(btf_id, 0, "find_prog_btf_id"))
> + goto cleanup;
> +
> + fentry_fd = load_fentry(bpf_program__fd(skel->progs.bind4), btf_id);
> + if (!ASSERT_GE(fentry_fd, 0, "load_fentry"))
> + goto cleanup;
> +
> + /* Make sure bpf_obj_get_info_by_fd works correctly when attaching
> + * to another BPF program.
> + */
> +
> + if (!ASSERT_OK(bpf_obj_get_info_by_fd(fentry_fd, &info, &info_len),
> + "bpf_obj_get_info_by_fd"))
> + goto cleanup;
> +
Conditional checking may not be necessary. Just
ASSERT_OK(bpf_obj_get_info_by_fd(...)). Looks more compact.
> + ASSERT_EQ(info.btf_id, 0, "info.btf_id");
> + ASSERT_GT(info.attach_btf_id, 0, "info.attach_btf_id");
> + ASSERT_GT(info.attach_btf_obj_id, 0, "info.attach_btf_obj_id");
> +
> +cleanup:
> + close(cgroup_fd);
> + close(fentry_fd);
> + attach_to_bpf__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/attach_to_bpf.c b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
> new file mode 100644
> index 000000000000..3f111fe96f8f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +SEC("cgroup/bind4")
> +int bind4(struct bpf_sock_addr *ctx)
> +{
> + return 1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.37.1.455.g008518b4e5-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-07-30 0:56 ` Hao Luo
@ 2022-07-30 2:16 ` Stanislav Fomichev
0 siblings, 0 replies; 9+ messages in thread
From: Stanislav Fomichev @ 2022-07-30 2:16 UTC (permalink / raw)
To: Hao Luo
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, jolsa
On Fri, Jul 29, 2022 at 5:56 PM Hao Luo <haoluo@google.com> wrote:
>
> On Fri, Jul 29, 2022 at 5:08 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Apparently, no existing selftest covers it. Add a new one where
> > we load cgroup/bind4 program and attach fentry to it.
> > Calling bpf_obj_get_info_by_fd on the fentry program
> > should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
>
> Looks good to me overall, with a few nits.
Thank you for the review! Will respin on Mon.
> > .../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
> > .../selftests/bpf/progs/attach_to_bpf.c | 12 ++
> > 2 files changed, 121 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > new file mode 100644
> > index 000000000000..fcf726c5ff0f
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#define _GNU_SOURCE
> > +#include <stdlib.h>
> > +#include <bpf/btf.h>
> > +#include <test_progs.h>
> > +#include <network_helpers.h>
> > +#include "attach_to_bpf.skel.h"
> > +
> > +char bpf_log_buf[BPF_LOG_BUF_SIZE];
> > +
> > +static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > +{
> > + struct bpf_prog_info info = {};
> > + __u32 info_len = sizeof(info);
> > + struct btf *btf;
> > + int err;
> > +
> > + err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
> > + if (err)
> > + return err;
> > +
> > + if (!info.btf_id)
> > + return -EINVAL;
> > +
> > + btf = btf__load_from_kernel_by_id(info.btf_id);
> > + err = libbpf_get_error(btf);
> > + if (err)
> > + return err;
> > +
> > + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
>
> nit: Prefer use a separate variable like 'btf_id' instead of using 'err'.
SG!
> > + btf__free(btf);
> > + if (err <= 0)
> > + return err;
> > +
> > + return err;
> > +}
> > +
> > +int load_fentry(int attach_prog_fd, int attach_btf_id)
> > +{
> > + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> > + .expected_attach_type = BPF_TRACE_FENTRY,
> > + .attach_prog_fd = attach_prog_fd,
> > + .attach_btf_id = attach_btf_id,
> > + .log_buf = bpf_log_buf,
> > + .log_size = sizeof(bpf_log_buf),
> > + );
> > + struct bpf_insn insns[] = {
> > + BPF_MOV64_IMM(BPF_REG_0, 0),
> > + BPF_EXIT_INSN(),
> > + };
> > + int ret;
> > +
> > + ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
> > + "bind4_fentry",
> > + "GPL",
> > + insns,
> > + ARRAY_SIZE(insns),
> > + &opts);
> > + if (ret)
> > + printf("verifier log: %s\n", bpf_log_buf);
>
> Do we need to print log? If print logging isn't necessary, we could
> directly return bpf_prog_load(). Seems simpler.
We don't really need it. I've been using it during development, but I
guess it should be fine to drop since the test is passing now :-)
> > + return ret;
> > +}
> > +
> > +void test_attach_to_bpf(void)
> > +{
> > + struct attach_to_bpf *skel = NULL;
> > + struct bpf_prog_info info = {};
> > + __u32 info_len = sizeof(info);
> > + int cgroup_fd = -1;
> > + int fentry_fd = -1;
> > + int btf_id;
> > +
> > + cgroup_fd = test__join_cgroup("/attach_to_bpf");
> > + if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
> > + return;
> > +
> > + skel = attach_to_bpf__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "skel"))
> > + goto cleanup;
> > +
> > + skel->links.bind4 = bpf_program__attach_cgroup(skel->progs.bind4, cgroup_fd);
> > + if (!ASSERT_OK_PTR(skel, "bpf_program__attach_cgroup"))
> > + goto cleanup;
> > +
> > + btf_id = find_prog_btf_id("bind4", bpf_program__fd(skel->progs.bind4));
> > + if (!ASSERT_GE(btf_id, 0, "find_prog_btf_id"))
> > + goto cleanup;
> > +
> > + fentry_fd = load_fentry(bpf_program__fd(skel->progs.bind4), btf_id);
> > + if (!ASSERT_GE(fentry_fd, 0, "load_fentry"))
> > + goto cleanup;
> > +
> > + /* Make sure bpf_obj_get_info_by_fd works correctly when attaching
> > + * to another BPF program.
> > + */
> > +
> > + if (!ASSERT_OK(bpf_obj_get_info_by_fd(fentry_fd, &info, &info_len),
> > + "bpf_obj_get_info_by_fd"))
> > + goto cleanup;
> > +
>
> Conditional checking may not be necessary. Just
> ASSERT_OK(bpf_obj_get_info_by_fd(...)). Looks more compact.
Ack, makes sense.
> > + ASSERT_EQ(info.btf_id, 0, "info.btf_id");
> > + ASSERT_GT(info.attach_btf_id, 0, "info.attach_btf_id");
> > + ASSERT_GT(info.attach_btf_obj_id, 0, "info.attach_btf_obj_id");
> > +
> > +cleanup:
> > + close(cgroup_fd);
> > + close(fentry_fd);
> > + attach_to_bpf__destroy(skel);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/attach_to_bpf.c b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
> > new file mode 100644
> > index 000000000000..3f111fe96f8f
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +
> > +SEC("cgroup/bind4")
> > +int bind4(struct bpf_sock_addr *ctx)
> > +{
> > + return 1;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > --
> > 2.37.1.455.g008518b4e5-goog
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id
2022-07-30 0:08 [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Stanislav Fomichev
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
2022-07-30 0:24 ` [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Hao Luo
@ 2022-08-01 16:57 ` Martin KaFai Lau
2 siblings, 0 replies; 9+ messages in thread
From: Martin KaFai Lau @ 2022-08-01 16:57 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Fri, Jul 29, 2022 at 05:08:08PM -0700, Stanislav Fomichev wrote:
> When attaching to program, the program itself might not be attached
> to anything (and, hence, might not have attach_btf), so we can't
> unconditionally use 'prog->aux->dst_prog->aux->attach_btf'.
> Instead, use bpf_prog_get_target_btf to pick proper target btf:
>
> * when attached to dst_prog, use dst_prog->aux->btf
> * when attached to kernel btf, use prog->aux->attach_btf
>
> Fixes: b79c9fc9551b ("bpf: implement BPF_PROG_QUERY for BPF_LSM_CGROUP")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> kernel/bpf/syscall.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 83c7136c5788..7dc3f8003631 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3886,6 +3886,7 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> union bpf_attr __user *uattr)
> {
> struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
> + struct btf *attach_btf = bpf_prog_get_target_btf(prog);
> struct bpf_prog_info info;
> u32 info_len = attr->info.info_len;
> struct bpf_prog_kstats stats;
> @@ -4088,10 +4089,8 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> if (prog->aux->btf)
> info.btf_id = btf_obj_id(prog->aux->btf);
> info.attach_btf_id = prog->aux->attach_btf_id;
> - if (prog->aux->attach_btf)
> - info.attach_btf_obj_id = btf_obj_id(prog->aux->attach_btf);
> - else if (prog->aux->dst_prog)
> - info.attach_btf_obj_id = btf_obj_id(prog->aux->dst_prog->aux->attach_btf);
> + if (attach_btf)
> + info.attach_btf_obj_id = btf_obj_id(attach_btf);
Nice.
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
2022-07-30 0:56 ` Hao Luo
@ 2022-08-01 17:45 ` Martin KaFai Lau
2022-08-01 18:23 ` Stanislav Fomichev
1 sibling, 1 reply; 9+ messages in thread
From: Martin KaFai Lau @ 2022-08-01 17:45 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Fri, Jul 29, 2022 at 05:08:09PM -0700, Stanislav Fomichev wrote:
> Apparently, no existing selftest covers it. Add a new one where
> we load cgroup/bind4 program and attach fentry to it.
> Calling bpf_obj_get_info_by_fd on the fentry program
> should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> .../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
> .../selftests/bpf/progs/attach_to_bpf.c | 12 ++
> 2 files changed, 121 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> new file mode 100644
> index 000000000000..fcf726c5ff0f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include <bpf/btf.h>
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +#include "attach_to_bpf.skel.h"
> +
> +char bpf_log_buf[BPF_LOG_BUF_SIZE];
static
> +
> +static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> +{
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + struct btf *btf;
> + int err;
> +
> + err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
> + if (err)
> + return err;
> +
> + if (!info.btf_id)
> + return -EINVAL;
> +
> + btf = btf__load_from_kernel_by_id(info.btf_id);
> + err = libbpf_get_error(btf);
> + if (err)
> + return err;
> +
> + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> + btf__free(btf);
> + if (err <= 0)
> + return err;
> +
> + return err;
> +}
> +
> +int load_fentry(int attach_prog_fd, int attach_btf_id)
static
> +{
> + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> + .expected_attach_type = BPF_TRACE_FENTRY,
> + .attach_prog_fd = attach_prog_fd,
> + .attach_btf_id = attach_btf_id,
> + .log_buf = bpf_log_buf,
> + .log_size = sizeof(bpf_log_buf),
> + );
> + struct bpf_insn insns[] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + int ret;
> +
> + ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
> + "bind4_fentry",
> + "GPL",
> + insns,
> + ARRAY_SIZE(insns),
> + &opts);
> + if (ret)
> + printf("verifier log: %s\n", bpf_log_buf);
If this fentry prog is in the attach_to_bpf.c and load by skel, this printf
and the bpf_log_buf can go away. I wonder if it can use the '?' like
SEC("?cgroup/bind4") and SEC("?fentry"). Then opens attach_to_bpf.skel.h
twice and use bpf_program__set_autoload() to load individual program.
Another option could be to reuse the progs/bind4_prog.c and directly
put the fentry program in the attach_to_bpf.c.
btw, this test feels like something that could be a few line
addition to the test_fexit_bpf2bpf_common() in fexit_bpf2bpf.c.
Adding one to test fentry into a cgroup bpf prog is also good.
No strong opinion here also.
> + return ret;
> +}
> +
> +void test_attach_to_bpf(void)
> +{
> + struct attach_to_bpf *skel = NULL;
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + int cgroup_fd = -1;
> + int fentry_fd = -1;
> + int btf_id;
> +
> + cgroup_fd = test__join_cgroup("/attach_to_bpf");
> + if (!ASSERT_GE(cgroup_fd, 0, "cgroup_fd"))
> + return;
> +
> + skel = attach_to_bpf__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel"))
> + goto cleanup;
> +
> + skel->links.bind4 = bpf_program__attach_cgroup(skel->progs.bind4, cgroup_fd);
> + if (!ASSERT_OK_PTR(skel, "bpf_program__attach_cgroup"))
> + goto cleanup;
> +
> + btf_id = find_prog_btf_id("bind4", bpf_program__fd(skel->progs.bind4));
> + if (!ASSERT_GE(btf_id, 0, "find_prog_btf_id"))
> + goto cleanup;
> +
> + fentry_fd = load_fentry(bpf_program__fd(skel->progs.bind4), btf_id);
> + if (!ASSERT_GE(fentry_fd, 0, "load_fentry"))
> + goto cleanup;
> +
> + /* Make sure bpf_obj_get_info_by_fd works correctly when attaching
> + * to another BPF program.
> + */
> +
> + if (!ASSERT_OK(bpf_obj_get_info_by_fd(fentry_fd, &info, &info_len),
> + "bpf_obj_get_info_by_fd"))
> + goto cleanup;
> +
> + ASSERT_EQ(info.btf_id, 0, "info.btf_id");
> + ASSERT_GT(info.attach_btf_id, 0, "info.attach_btf_id");
> + ASSERT_GT(info.attach_btf_obj_id, 0, "info.attach_btf_obj_id");
> +
> +cleanup:
> + close(cgroup_fd);
> + close(fentry_fd);
> + attach_to_bpf__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/attach_to_bpf.c b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
> new file mode 100644
> index 000000000000..3f111fe96f8f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/attach_to_bpf.c
nit. attach_to_bpf.c sounds too broad.
May be fentry_to_cgroup_bpf.c ?
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +SEC("cgroup/bind4")
> +int bind4(struct bpf_sock_addr *ctx)
> +{
> + return 1;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.37.1.455.g008518b4e5-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-08-01 17:45 ` Martin KaFai Lau
@ 2022-08-01 18:23 ` Stanislav Fomichev
2022-08-01 19:55 ` Martin KaFai Lau
0 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2022-08-01 18:23 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Mon, Aug 1, 2022 at 10:45 AM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Fri, Jul 29, 2022 at 05:08:09PM -0700, Stanislav Fomichev wrote:
> > Apparently, no existing selftest covers it. Add a new one where
> > we load cgroup/bind4 program and attach fentry to it.
> > Calling bpf_obj_get_info_by_fd on the fentry program
> > should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
> > .../selftests/bpf/progs/attach_to_bpf.c | 12 ++
> > 2 files changed, 121 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > new file mode 100644
> > index 000000000000..fcf726c5ff0f
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#define _GNU_SOURCE
> > +#include <stdlib.h>
> > +#include <bpf/btf.h>
> > +#include <test_progs.h>
> > +#include <network_helpers.h>
> > +#include "attach_to_bpf.skel.h"
> > +
> > +char bpf_log_buf[BPF_LOG_BUF_SIZE];
> static
Will remove bpf_log_buf. Sent v2 too soon :-(
> > +
> > +static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > +{
> > + struct bpf_prog_info info = {};
> > + __u32 info_len = sizeof(info);
> > + struct btf *btf;
> > + int err;
> > +
> > + err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
> > + if (err)
> > + return err;
> > +
> > + if (!info.btf_id)
> > + return -EINVAL;
> > +
> > + btf = btf__load_from_kernel_by_id(info.btf_id);
> > + err = libbpf_get_error(btf);
> > + if (err)
> > + return err;
> > +
> > + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > + btf__free(btf);
> > + if (err <= 0)
> > + return err;
> > +
> > + return err;
> > +}
> > +
> > +int load_fentry(int attach_prog_fd, int attach_btf_id)
> static
Thx!
> > +{
> > + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> > + .expected_attach_type = BPF_TRACE_FENTRY,
> > + .attach_prog_fd = attach_prog_fd,
> > + .attach_btf_id = attach_btf_id,
> > + .log_buf = bpf_log_buf,
> > + .log_size = sizeof(bpf_log_buf),
> > + );
> > + struct bpf_insn insns[] = {
> > + BPF_MOV64_IMM(BPF_REG_0, 0),
> > + BPF_EXIT_INSN(),
> > + };
> > + int ret;
> > +
> > + ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
> > + "bind4_fentry",
> > + "GPL",
> > + insns,
> > + ARRAY_SIZE(insns),
> > + &opts);
> > + if (ret)
> > + printf("verifier log: %s\n", bpf_log_buf);
> If this fentry prog is in the attach_to_bpf.c and load by skel, this printf
> and the bpf_log_buf can go away. I wonder if it can use the '?' like
> SEC("?cgroup/bind4") and SEC("?fentry"). Then opens attach_to_bpf.skel.h
> twice and use bpf_program__set_autoload() to load individual program .
Good ideal, let me try to see if doing "?fentry" is easier..
(unless we agree to keep load_fentry, see below)
> Another option could be to reuse the progs/bind4_prog.c and directly
> put the fentry program in the attach_to_bpf.c.
>
> btw, this test feels like something that could be a few line
> addition to the test_fexit_bpf2bpf_common() in fexit_bpf2bpf.c.
> Adding one to test fentry into a cgroup bpf prog is also good.
> No strong opinion here also.
I was trying to reuse fexit_bpf2bpf initially but I sank too much time
into it and decided that it might be easier to write a
simpler/separate reproducer instead :-(
How about we reuse progs/bind4_prog.c and keep load_fentry() ? And put
this new test in fexit_bpf2bpf.c ?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf
2022-08-01 18:23 ` Stanislav Fomichev
@ 2022-08-01 19:55 ` Martin KaFai Lau
0 siblings, 0 replies; 9+ messages in thread
From: Martin KaFai Lau @ 2022-08-01 19:55 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa
On Mon, Aug 01, 2022 at 11:23:56AM -0700, Stanislav Fomichev wrote:
> On Mon, Aug 1, 2022 at 10:45 AM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Fri, Jul 29, 2022 at 05:08:09PM -0700, Stanislav Fomichev wrote:
> > > Apparently, no existing selftest covers it. Add a new one where
> > > we load cgroup/bind4 program and attach fentry to it.
> > > Calling bpf_obj_get_info_by_fd on the fentry program
> > > should return non-zero btf_id/btf_obj_id instead of crashing the kernel.
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > > .../selftests/bpf/prog_tests/attach_to_bpf.c | 109 ++++++++++++++++++
> > > .../selftests/bpf/progs/attach_to_bpf.c | 12 ++
> > > 2 files changed, 121 insertions(+)
> > > create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/attach_to_bpf.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > > new file mode 100644
> > > index 000000000000..fcf726c5ff0f
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_to_bpf.c
> > > @@ -0,0 +1,109 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#define _GNU_SOURCE
> > > +#include <stdlib.h>
> > > +#include <bpf/btf.h>
> > > +#include <test_progs.h>
> > > +#include <network_helpers.h>
> > > +#include "attach_to_bpf.skel.h"
> > > +
> > > +char bpf_log_buf[BPF_LOG_BUF_SIZE];
> > static
>
> Will remove bpf_log_buf. Sent v2 too soon :-(
>
> > > +
> > > +static int find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > > +{
> > > + struct bpf_prog_info info = {};
> > > + __u32 info_len = sizeof(info);
> > > + struct btf *btf;
> > > + int err;
> > > +
> > > + err = bpf_obj_get_info_by_fd(attach_prog_fd, &info, &info_len);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (!info.btf_id)
> > > + return -EINVAL;
> > > +
> > > + btf = btf__load_from_kernel_by_id(info.btf_id);
> > > + err = libbpf_get_error(btf);
> > > + if (err)
> > > + return err;
> > > +
> > > + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > > + btf__free(btf);
> > > + if (err <= 0)
> > > + return err;
> > > +
> > > + return err;
> > > +}
> > > +
> > > +int load_fentry(int attach_prog_fd, int attach_btf_id)
> > static
>
> Thx!
>
> > > +{
> > > + LIBBPF_OPTS(bpf_prog_load_opts, opts,
> > > + .expected_attach_type = BPF_TRACE_FENTRY,
> > > + .attach_prog_fd = attach_prog_fd,
> > > + .attach_btf_id = attach_btf_id,
> > > + .log_buf = bpf_log_buf,
> > > + .log_size = sizeof(bpf_log_buf),
> > > + );
> > > + struct bpf_insn insns[] = {
> > > + BPF_MOV64_IMM(BPF_REG_0, 0),
> > > + BPF_EXIT_INSN(),
> > > + };
> > > + int ret;
> > > +
> > > + ret = bpf_prog_load(BPF_PROG_TYPE_TRACING,
> > > + "bind4_fentry",
> > > + "GPL",
> > > + insns,
> > > + ARRAY_SIZE(insns),
> > > + &opts);
> > > + if (ret)
> > > + printf("verifier log: %s\n", bpf_log_buf);
> > If this fentry prog is in the attach_to_bpf.c and load by skel, this printf
> > and the bpf_log_buf can go away. I wonder if it can use the '?' like
> > SEC("?cgroup/bind4") and SEC("?fentry"). Then opens attach_to_bpf.skel.h
> > twice and use bpf_program__set_autoload() to load individual program .
>
> Good ideal, let me try to see if doing "?fentry" is easier..
> (unless we agree to keep load_fentry, see below)
>
> > Another option could be to reuse the progs/bind4_prog.c and directly
> > put the fentry program in the attach_to_bpf.c.
> >
> > btw, this test feels like something that could be a few line
> > addition to the test_fexit_bpf2bpf_common() in fexit_bpf2bpf.c.
> > Adding one to test fentry into a cgroup bpf prog is also good.
> > No strong opinion here also.
>
> I was trying to reuse fexit_bpf2bpf initially but I sank too much time
> into it and decided that it might be easier to write a
> simpler/separate reproducer instead :-(
> How about we reuse progs/bind4_prog.c and keep load_fentry() ? And put
> this new test in fexit_bpf2bpf.c ?
Reusing bind4_prog.c sounds good. Either use bind4_prog.c in the
new prog_tests/attach_to_bpf.c (likely need a better name) or the
fexit_bpf2bpf.c is fine. Whatever makes more sense.
I was mostly thinking to avoid the special verifier log buf handling and
the printf here. If an empty fentry prog may look weird, may be just
skip the bpf_log_buf and printf since it will never fail and keep the
load_fentry ?
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-01 19:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-30 0:08 [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Stanislav Fomichev
2022-07-30 0:08 ` [PATCH bpf-next 2/2] selftests/bpf: Excercise bpf_obj_get_info_by_fd for bpf2bpf Stanislav Fomichev
2022-07-30 0:56 ` Hao Luo
2022-07-30 2:16 ` Stanislav Fomichev
2022-08-01 17:45 ` Martin KaFai Lau
2022-08-01 18:23 ` Stanislav Fomichev
2022-08-01 19:55 ` Martin KaFai Lau
2022-07-30 0:24 ` [PATCH bpf-next 1/2] bpf: use proper target btf when exporting attach_btf_obj_id Hao Luo
2022-08-01 16:57 ` Martin KaFai Lau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.