On Wed, Jul 6, 2022 at 12:08 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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
