From: Jiri Olsa <olsajiri@gmail.com>
To: Dmitrii Dolgov <9erthalion6@gmail.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
yonghong.song@linux.dev, dan.carpenter@linaro.org,
olsajiri@gmail.com
Subject: Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach
Date: Thu, 30 Nov 2023 16:14:55 +0100 [thread overview]
Message-ID: <ZWim7zRLA-cgVQpr@krava> (raw)
In-Reply-To: <20231129195240.19091-4-9erthalion6@gmail.com>
On Wed, Nov 29, 2023 at 08:52:38PM +0100, Dmitrii Dolgov wrote:
> It looks like there is an issue in bpf_tracing_prog_attach, in the
> "prog->aux->dst_trampoline and tgt_prog is NULL" case. One can construct
> a sequence of events when prog->aux->attach_btf will be NULL, and
> bpf_trampoline_compute_key will fail.
>
> BUG: kernel NULL pointer dereference, address: 0000000000000058
> Call Trace:
> <TASK>
> ? __die+0x20/0x70
> ? page_fault_oops+0x15b/0x430
> ? fixup_exception+0x22/0x330
> ? exc_page_fault+0x6f/0x170
> ? asm_exc_page_fault+0x22/0x30
> ? bpf_tracing_prog_attach+0x279/0x560
> ? btf_obj_id+0x5/0x10
> bpf_tracing_prog_attach+0x439/0x560
> __sys_bpf+0x1cf4/0x2de0
> __x64_sys_bpf+0x1c/0x30
> do_syscall_64+0x41/0xf0
> entry_SYSCALL_64_after_hwframe+0x6e/0x76
>
> The issue seems to be not relevant to the previous changes with
> recursive tracing prog attach, because the reproducing test doesn't
> actually include recursive fentry attaching.
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
> kernel/bpf/syscall.c | 4 +-
> .../bpf/prog_tests/recursive_attach.c | 48 +++++++++++++++++++
> .../bpf/progs/fentry_recursive_target.c | 11 +++++
> 3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a595d7a62dbc..e01a949dfed7 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3197,7 +3197,9 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> goto out_unlock;
> }
> btf_id = prog->aux->attach_btf_id;
> - key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
> + if (prog->aux->attach_btf)
> + key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf,
> + btf_id);
> }
nice catch.. I'd think dst_trampoline would caught it, because the
program is loaded with attach_prog_fd=x and check_attach_btf_id should
create dst_trampoline.. hum
jirka
>
> if (!prog->aux->dst_trampoline ||
> diff --git a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> index 9c422dd92c4e..a4abf1745e62 100644
> --- a/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/recursive_attach.c
> @@ -83,3 +83,51 @@ void test_recursive_fentry_attach(void)
> fentry_recursive__destroy(tracing_chain[i]);
> }
> }
> +
> +/*
> + * Test that a tracing prog reattachment (when we land in
> + * "prog->aux->dst_trampoline and tgt_prog is NULL" branch in
> + * bpf_tracing_prog_attach) does not lead to a crash due to missing attach_btf
> + */
> +void test_fentry_attach_btf_presence(void)
> +{
> + struct fentry_recursive_target *target_skel = NULL;
> + struct fentry_recursive *tracing_skel = NULL;
> + struct bpf_program *prog;
> + int err, link_fd, tgt_prog_fd;
> +
> + target_skel = fentry_recursive_target__open_and_load();
> + if (!ASSERT_OK_PTR(target_skel, "fentry_recursive_target__open_and_load"))
> + goto close_prog;
> +
> + tracing_skel = fentry_recursive__open();
> + if (!ASSERT_OK_PTR(tracing_skel, "fentry_recursive__open"))
> + goto close_prog;
> +
> + prog = tracing_skel->progs.recursive_attach;
> + tgt_prog_fd = bpf_program__fd(target_skel->progs.fentry_target);
> + err = bpf_program__set_attach_target(prog, tgt_prog_fd, "fentry_target");
> + if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
> + goto close_prog;
> +
> + err = fentry_recursive__load(tracing_skel);
> + if (!ASSERT_OK(err, "fentry_recursive__load"))
> + goto close_prog;
> +
> + LIBBPF_OPTS(bpf_link_create_opts, link_opts);
> +
> + link_fd = bpf_link_create(bpf_program__fd(tracing_skel->progs.recursive_attach),
> + 0, BPF_TRACE_FENTRY, &link_opts);
> + if (!ASSERT_GE(link_fd, 0, "link_fd"))
> + goto close_prog;
> +
> + fentry_recursive__detach(tracing_skel);
> +
> + err = fentry_recursive__attach(tracing_skel);
> + if (!ASSERT_ERR(err, "fentry_recursive__attach"))
> + goto close_prog;
> +
> +close_prog:
> + fentry_recursive_target__destroy(target_skel);
> + fentry_recursive__destroy(tracing_skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/fentry_recursive_target.c b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
> index b6fb8ebd598d..f812d2de0c3c 100644
> --- a/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
> +++ b/tools/testing/selftests/bpf/progs/fentry_recursive_target.c
> @@ -18,3 +18,14 @@ int BPF_PROG(test1, int a)
> test1_result = a == 1;
> return 0;
> }
> +
> +/*
> + * Dummy bpf prog for testing attach_btf presence when attaching an fentry
> + * program.
> + */
> +SEC("raw_tp/sys_enter")
> +int BPF_PROG(fentry_target, struct pt_regs *regs, long id)
> +{
> + test1_result = id == 1;
> + return 0;
> +}
> --
> 2.41.0
>
next prev parent reply other threads:[~2023-11-30 15:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-29 19:52 [PATCH bpf-next v4 0/3] Relax tracing prog recursive attach rules Dmitrii Dolgov
2023-11-29 19:52 ` [PATCH bpf-next v4 1/3] bpf: " Dmitrii Dolgov
2023-11-29 23:58 ` Song Liu
2023-11-30 10:08 ` Dmitry Dolgov
2023-11-30 20:19 ` Song Liu
2023-11-30 20:41 ` Dmitry Dolgov
2023-12-01 9:55 ` Artem Savkov
2023-12-01 14:29 ` Dmitry Dolgov
2023-11-30 14:30 ` Jiri Olsa
2023-11-30 18:57 ` Dmitry Dolgov
2023-11-30 22:34 ` Jiri Olsa
2023-11-29 19:52 ` [PATCH bpf-next v4 2/3] selftests/bpf: Add test for recursive attachment of tracing progs Dmitrii Dolgov
2023-11-30 14:47 ` Jiri Olsa
2023-11-29 19:52 ` [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach Dmitrii Dolgov
2023-11-30 15:14 ` Jiri Olsa [this message]
2023-11-30 22:30 ` Jiri Olsa
2023-12-01 14:21 ` Dmitry Dolgov
2023-12-01 14:52 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZWim7zRLA-cgVQpr@krava \
--to=olsajiri@gmail.com \
--cc=9erthalion6@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=dan.carpenter@linaro.org \
--cc=daniel@iogearbox.net \
--cc=martin.lau@linux.dev \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.