| Message ID | 20220704140850.1106119-1-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Error out when missing binary_path for USDT attach | expand |
On Mon, Jul 4, 2022 at 7:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > The binary_path parameter is required for bpf_program__attach_usdt(). > Error out when user attach USDT probe without specifying a binary_path. > This is a required parameter, libbpf doesn't add pr_warn() for every `const char *` parameter that the user incorrectly passes NULL for (e.g., bpf_program__attach_kprobe's func_name). If you think bpf_program__attach_usdt() doc comment about this is not clear enough, let's improve the documentation instead of littering libbpf source code with Java-like NULL checks everywhere. > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > tools/lib/bpf/libbpf.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8a45a84eb9b2..5e4153c5b0a6 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -10686,6 +10686,12 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, > return libbpf_err_ptr(-EINVAL); > } > > + if (!binary_path) { > + pr_warn("prog '%s': USDT attach requires binary_path\n", > + prog->name); > + return libbpf_err_ptr(-EINVAL); > + } > + > if (!strchr(binary_path, '/')) { > err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); > if (err) { > -- > 2.30.2
Hi, Andrii On 2022/7/6 13:05, Andrii Nakryiko wrote: > On Mon, Jul 4, 2022 at 7:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> The binary_path parameter is required for bpf_program__attach_usdt(). >> Error out when user attach USDT probe without specifying a binary_path. >> > > This is a required parameter, libbpf doesn't add pr_warn() for every > `const char *` parameter that the user incorrectly passes NULL for > (e.g., bpf_program__attach_kprobe's func_name). If you think I understand this is a required parameter. The intention of this patch is to avoid coredump if user passes NULL for binary_path argument, not just emit a warning. The uprobe handling code of libbpf already did this. BTW, most of libbpf APIs do NULL check for their const char * parameters and return -EINVAL. > bpf_program__attach_usdt() doc comment about this is not clear enough, > let's improve the documentation instead of littering libbpf source > code with Java-like NULL checks everywhere. > >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- >> tools/lib/bpf/libbpf.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 8a45a84eb9b2..5e4153c5b0a6 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -10686,6 +10686,12 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, >> return libbpf_err_ptr(-EINVAL); >> } >> >> + if (!binary_path) { >> + pr_warn("prog '%s': USDT attach requires binary_path\n", >> + prog->name); >> + return libbpf_err_ptr(-EINVAL); >> + } >> + >> if (!strchr(binary_path, '/')) { >> err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); >> if (err) { >> -- >> 2.30.2 -- Hengqi
On Wed, Jul 6, 2022 at 12:08 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > Hi, Andrii > > On 2022/7/6 13:05, Andrii Nakryiko wrote: > > On Mon, Jul 4, 2022 at 7:09 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > >> > >> The binary_path parameter is required for bpf_program__attach_usdt(). > >> Error out when user attach USDT probe without specifying a binary_path. > >> > > > > This is a required parameter, libbpf doesn't add pr_warn() for every > > `const char *` parameter that the user incorrectly passes NULL for > > (e.g., bpf_program__attach_kprobe's func_name). If you think > > I understand this is a required parameter. The intention of this patch is > to avoid coredump if user passes NULL for binary_path argument, not just > emit a warning. The uprobe handling code of libbpf already did this. > > BTW, most of libbpf APIs do NULL check for their const char * parameters > and return -EINVAL. Even some of pretty old APIs like bpf_program__pin() don't do that for path. But ok, given bpf_program__attach_uprobe_opts() checks binary_path for NULL, let's add check and return -EINVAL. But let's skip pr_warn(). And while you are at it, can you move the binary_path check in attach_uprobe_opts up, it's weirdly nested in func_name check, not sure why is that, tbh. I'm not sure uprobe attach can even succeed with NULL binary_path, so it's weird that we don't always reject it. > > > bpf_program__attach_usdt() doc comment about this is not clear enough, > > let's improve the documentation instead of littering libbpf source > > code with Java-like NULL checks everywhere. > > > >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >> --- > >> tools/lib/bpf/libbpf.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 8a45a84eb9b2..5e4153c5b0a6 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -10686,6 +10686,12 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, > >> return libbpf_err_ptr(-EINVAL); > >> } > >> > >> + if (!binary_path) { > >> + pr_warn("prog '%s': USDT attach requires binary_path\n", > >> + prog->name); > >> + return libbpf_err_ptr(-EINVAL); > >> + } > >> + > >> if (!strchr(binary_path, '/')) { > >> err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); > >> if (err) { > >> -- > >> 2.30.2 > > > -- > Hengqi
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 8a45a84eb9b2..5e4153c5b0a6 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -10686,6 +10686,12 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog, return libbpf_err_ptr(-EINVAL); } + if (!binary_path) { + pr_warn("prog '%s': USDT attach requires binary_path\n", + prog->name); + return libbpf_err_ptr(-EINVAL); + } + if (!strchr(binary_path, '/')) { err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path)); if (err) {
The binary_path parameter is required for bpf_program__attach_usdt(). Error out when user attach USDT probe without specifying a binary_path. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/lib/bpf/libbpf.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.30.2