On Tue, 20 Dec 2022 15:53:18 -0800 Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> On Mon, Dec 19, 2022 at 5:57 PM Xin Liu <liuxin350@xxxxxxxxxx> wrote:
> >
> > On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@xxxxxxxxxx> wrote:
> > > On 12/19, Xin Liu wrote:
> > > > The API functions bpf_program__attach_perf_event_opts and
> > > > bpf_program_attach_usdt can be invoked by users. However, when the
> > > > input prog parameter is null, the API uses name and obj without
> > > > check. This will cause program to crash directly.
> > >
> > > Why do we care about these only? We have a lot of functions invoked
> > > by the users which don't check the arguments. Can the caller ensure
> > > the prog is valid/consistent before calling these?
> > >
> >
> > Thanks to sdf for this suggestions.
> >
> > But I don't think it's a good idea to let the user guarantee:
> > 1.We can't require all users to verify parameters before transferring
> > parameters. Some parameters may be omitted. If the user forgets to check
> > the program pointer and it happens to be NULL, the program will crash
> > without any last words, and the user can only use the debugging tool to
> > collect relevant clues, which is a disaster for the user.
> > 2.Code changes are required for completed user programs and places where
> > the API is invoked. For users, the cost of ensuring that each parameter
> > check result is correct is high, which is much higher than that of
> > directly verifying the parameter in libbpf.
> >
> > So I think we should do some validation at the API entrance, whick is a
> > big benefit at the minimum cost, and in fact we do that, for example,
> > OPTS_VALID validation, right?
> >
>
> I agree with Stanislav. There is no reason for user to assume that
> passing NULL works as a general rule. We do not check for NULL
> everywhere. If user doesn't follow API contract, yes, they will get
> crashes or confusing behavior, unfortunately.
>
> For APIs that explicitly allow passing NULL for strings, documentation
> clearly states that. And if not, we should improve documentation.
>
> > > > Signed-off-by: Xin Liu <liuxin350@xxxxxxxxxx>
> > > > ---
> > > > tools/lib/bpf/libbpf.c | 13 ++++++++++++-
> > > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 2a82f49ce16f..0d21de4f7d5c 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -9764,6 +9764,11 @@ struct bpf_link
> > > > *bpf_program__attach_perf_event_opts(const struct bpf_program *p
> > > > if (!OPTS_VALID(opts, bpf_perf_event_opts))
> > > > return libbpf_err_ptr(-EINVAL);
> > > >
> > > > + if (!prog || !prog->name) {
> > > > + pr_warn("prog: invalid prog\n");
> > > > + return libbpf_err_ptr(-EINVAL);
> > > > + }
> > > > +
> > > > if (pfd < 0) {
> > > > pr_warn("prog '%s': invalid perf event FD %d\n",
> > > > prog->name, pfd);
> > > > @@ -10967,7 +10972,7 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > > const struct bpf_usdt_opts *opts)
> > > > {
> > > > char resolved_path[512];
> > > > - struct bpf_object *obj = prog->obj;
> > > > + struct bpf_object *obj;
> > > > struct bpf_link *link;
> > > > __u64 usdt_cookie;
> > > > int err;
> > > > @@ -10975,6 +10980,11 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > > if (!OPTS_VALID(opts, bpf_uprobe_opts))
> > > > return libbpf_err_ptr(-EINVAL);
> > > >
> > > > + if (!prog || !prog->name || !prog->obj) {
> > > > + pr_warn("prog: invalid prog\n");
> > > > + return libbpf_err_ptr(-EINVAL);
> > > > + }
> > > > +
> > > > if (bpf_program__fd(prog) < 0) {
> > > > pr_warn("prog '%s': can't attach BPF program w/o FD (did you load
> > > > it?)\n",
> > > > prog->name);
> > > > @@ -10997,6 +11007,7 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > > /* USDT manager is instantiated lazily on first USDT attach. It will
> > > > * be destroyed together with BPF object in bpf_object__close().
> > > > */
> > > > + obj = prog->obj;
> > > > if (IS_ERR(obj->usdt_man))
> > > > return libbpf_ptr(obj->usdt_man);
> > > > if (!obj->usdt_man) {
> > > > --
> > > > 2.33.0
Thanks to Andrii and sdf.
I will resubmit a patch to update the API documentation.
