| Message ID | 20220630135250.241795-1-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Allow attach USDT BPF program without specifying binary path | expand |
Hi Hengqi, On Thu, Jun 30, 2022 at 7:07 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Currently, libbpf requires specifying binary path when attach USDT BPF program > manually. This is not necessary because we can infer that from /proc/$PID/exe. > This also avoids coredump when user do not provide binary path. > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > tools/lib/bpf/libbpf.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8a45a84eb9b2..4ee9b6a0944e 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10686,7 +10686,19 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, > return libbpf_err_ptr(-EINVAL); > } > > - if (!strchr(binary_path, '/')) { > + if (!binary_path) { > + if (pid < 0) { > + pr_warn("prog '%s': missing attach target, pid or binary path required\n", > + prog->name); > + return libbpf_err_ptr(-EINVAL); > + } > + if (!pid) > + binary_path = "/proc/self/exe"; > + else { > + snprintf(resolved_path, sizeof(resolved_path), "/proc/%d/exe", pid); > + binary_path = resolved_path; > + } Please add matching brackets for the 'then' clause. Besides, do you need to call readlink() to extract the real path of the binary? Reading /proc/$pid/exe may fail due to not being root. Detecting such failures early and giving a warning would be great. > + } else if (!strchr(binary_path, '/')) { > err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); > if (err) { > pr_warn("prog '%s': failed to resolve full path for '%s': %d\n", > -- > 2.30.2 >
On Thu, Jun 30, 2022 at 6:53 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Currently, libbpf requires specifying binary path when attach USDT BPF program > manually. This is not necessary because we can infer that from /proc/$PID/exe. > This also avoids coredump when user do not provide binary path. > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- Hm... just because I specify PID, doesn't mean I mean main binary of that process, it could be some other shared library within that process. I don't really like this change because it doesn't feel 100% obvious and natural. User can easily specify "/proc/self/exe" or "/proc/<pid>/exe" if that's what they are targeting. Documentation for bpf_program__attach_usdt() states @param binary_path Path to binary that contains provided USDT probe it doesn't say it is optional and can be NULL, so there is no valid reason to pass NULL and expect things to work. > tools/lib/bpf/libbpf.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8a45a84eb9b2..4ee9b6a0944e 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10686,7 +10686,19 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, > return libbpf_err_ptr(-EINVAL); > } > > - if (!strchr(binary_path, '/')) { > + if (!binary_path) { > + if (pid < 0) { > + pr_warn("prog '%s': missing attach target, pid or binary path required\n", > + prog->name); > + return libbpf_err_ptr(-EINVAL); > + } > + if (!pid) > + binary_path = "/proc/self/exe"; > + else { > + snprintf(resolved_path, sizeof(resolved_path), "/proc/%d/exe", pid); > + binary_path = resolved_path; > + } > + } else if (!strchr(binary_path, '/')) { > err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); > if (err) { > pr_warn("prog '%s': failed to resolve full path for '%s': %d\n", > -- > 2.30.2 >
On 2022/7/1 03:20, Andrii Nakryiko wrote: > On Thu, Jun 30, 2022 at 6:53 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> Currently, libbpf requires specifying binary path when attach USDT BPF program >> manually. This is not necessary because we can infer that from /proc/$PID/exe. >> This also avoids coredump when user do not provide binary path. >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- > > Hm... just because I specify PID, doesn't mean I mean main binary of > that process, it could be some other shared library within that > process. > > I don't really like this change because it doesn't feel 100% obvious > and natural. User can easily specify "/proc/self/exe" or > "/proc/<pid>/exe" if that's what they are targeting. > Actually, this is an implication and practice set by BCC. If path is not set, BCC assumes that it is targeting the process. > Documentation for bpf_program__attach_usdt() states > > @param binary_path Path to binary that contains provided USDT probe > > it doesn't say it is optional and can be NULL, so there is no valid > reason to pass NULL and expect things to work. > > OK, then we should at least check against NULL and emit a warning. >> tools/lib/bpf/libbpf.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 8a45a84eb9b2..4ee9b6a0944e 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -10686,7 +10686,19 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, >> return libbpf_err_ptr(-EINVAL); >> } >> >> - if (!strchr(binary_path, '/')) { >> + if (!binary_path) { >> + if (pid < 0) { >> + pr_warn("prog '%s': missing attach target, pid or binary path required\n", >> + prog->name); >> + return libbpf_err_ptr(-EINVAL); >> + } >> + if (!pid) >> + binary_path = "/proc/self/exe"; >> + else { >> + snprintf(resolved_path, sizeof(resolved_path), "/proc/%d/exe", pid); >> + binary_path = resolved_path; >> + } >> + } else if (!strchr(binary_path, '/')) { >> err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); >> if (err) { >> pr_warn("prog '%s': failed to resolve full path for '%s': %d\n", >> -- >> 2.30.2 >>
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 8a45a84eb9b2..4ee9b6a0944e 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10686,7 +10686,19 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, return libbpf_err_ptr(-EINVAL); } - if (!strchr(binary_path, '/')) { + if (!binary_path) { + if (pid < 0) { + pr_warn("prog '%s': missing attach target, pid or binary path required\n", + prog->name); + return libbpf_err_ptr(-EINVAL); + } + if (!pid) + binary_path = "/proc/self/exe"; + else { + snprintf(resolved_path, sizeof(resolved_path), "/proc/%d/exe", pid); + binary_path = resolved_path; + } + } else if (!strchr(binary_path, '/')) { err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); if (err) { pr_warn("prog '%s': failed to resolve full path for '%s': %d\n",
Currently, libbpf requires specifying binary path when attach USDT BPF program manually. This is not necessary because we can infer that from /proc/$PID/exe. This also avoids coredump when user do not provide binary path. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/lib/bpf/libbpf.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) -- 2.30.2