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?
> > 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
