[DTrace-devel] [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes
Kris Van Hees
kris.van.hees at oracle.com
Fri Aug 2 17:52:31 UTC 2024
On Fri, Aug 02, 2024 at 01:45:46PM -0400, Eugene Loh wrote:
> Not a real careful review, since I'm not that familiar with the BTF stuff,
> but:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>
> I assume the new tests already pass on kprobes even without the new fixes.
>
> Might the tests be (theoretically) a little more stringent with .r files?
Hm, yes, good point. On that for v4.
> On 8/2/24 12:35, Kris Van Hees wrote:
>
> > We were trying to access a return value for void functions, causing BPF
> > verifier failures. We can determine the return type when getting the
> > probe info, and set the return probe argc value accordingly. This then
> > drives the code generation in the trampoline.
> >
> > Reported-by: Alan Maguire <alan.maguire at oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_btf.c | 16 +++++++++++++
> > libdtrace/dt_btf.h | 2 ++
> > libdtrace/dt_prov_fbt.c | 21 +++++++++-------
> > .../fbtprovider/err.D_ARGS_IDX.void-void.d | 24 +++++++++++++++++++
> > .../fbtprovider/err.D_ARGS_IDX.void.d | 24 +++++++++++++++++++
> > 5 files changed, 78 insertions(+), 9 deletions(-)
> > create mode 100644 test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> > create mode 100644 test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> >
> > diff --git a/libdtrace/dt_btf.c b/libdtrace/dt_btf.c
> > index 77366df6..e905580f 100644
> > --- a/libdtrace/dt_btf.c
> > +++ b/libdtrace/dt_btf.c
> > @@ -980,3 +980,19 @@ dt_btf_func_argc(dtrace_hdl_t *dtp, const dt_btf_t *btf, uint32_t id)
> > return -1;
> > }
> > +
> > +int
> > +dt_btf_func_is_void(dtrace_hdl_t *dtp, const dt_btf_t *btf, uint32_t id)
> > +{
> > + btf_type_t *type = dt_btf_type_by_id(dtp, btf, id);
> > +
> > + /* For functions, move on to the function prototype. */
> > + if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC)
> > + type = dt_btf_type_by_id(dtp, btf, type->type);
> > +
> > + if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC_PROTO &&
> > + type->type == 0)
> > + return 1;
> > +
> > + return 0;
> > +}
> > diff --git a/libdtrace/dt_btf.h b/libdtrace/dt_btf.h
> > index a7cee464..e05b30d0 100644
> > --- a/libdtrace/dt_btf.h
> > +++ b/libdtrace/dt_btf.h
> > @@ -25,6 +25,8 @@ extern int32_t dt_btf_lookup_name_kind(dtrace_hdl_t *, dt_btf_t *,
> > const char *, uint32_t);
> > extern int dt_btf_func_argc(dtrace_hdl_t *dtp, const dt_btf_t *btf,
> > uint32_t id);
> > +extern int dt_btf_func_is_void(dtrace_hdl_t *dtp, const dt_btf_t *btf,
> > + uint32_t id);
> > extern int dt_btf_get_module_ids(dtrace_hdl_t *);
> > extern int dt_btf_module_fd(const dt_module_t *);
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 461d5e35..80c3db20 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -199,13 +199,15 @@ static int fprobe_trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> > emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> > /*
> > + * Only try to retrieve a return value if we know we can.
> > + *
> > * The return value is provided by the fexit probe as an
> > * argument slot past the last function argument. We can get
> > * the number of function arguments using the BTF id that has
> > * been stored as the tracepoint event id.
> > */
> > dmp = dt_module_lookup_by_name(dtp, prp->desc->mod);
> > - if (dmp != NULL) {
> > + if (dmp && prp->argc == 2) {
> > int32_t btf_id = dt_tp_get_event_id(prp);
> > int i = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
> > @@ -240,7 +242,9 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > dt_tp_set_event_id(prp, btf_id);
> > if (strcmp(desc->prb, "return") == 0) {
> > - argc = 2;
> > + /* Void function return probes only provide 1 argument. */
> > + argc = dt_btf_func_is_void(dtp, dmp->dm_btf, btf_id) ? 1 : 2;
> > +
> > argv = dt_calloc(dtp, argc, sizeof(dt_argdesc_t));
> > if (argv == NULL)
> > return dt_set_errno(dtp, EDT_NOMEM);
> > @@ -250,13 +254,12 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > argv[0].native = strdup("uint64_t");
> > argv[0].xlate = NULL;
> > - /*
> > - * The return type is a generic uint64_t. For void functions
> > - * the value of arg1 and argv[1] is undefined.
> > - */
> > - argv[1].mapping = 1;
> > - argv[1].native = strdup("uint64_t");
> > - argv[1].xlate = NULL;
> > + if (argc == 2) {
> > + /* The return type is a generic uint64_t. */
> > + argv[1].mapping = 1;
> > + argv[1].native = strdup("uint64_t");
> > + argv[1].xlate = NULL;
> > + }
> > goto done;
> > }
> > diff --git a/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d b/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> > new file mode 100644
> > index 00000000..4bdf36cb
> > --- /dev/null
> > +++ b/test/unittest/fbtprovider/err.D_ARGS_IDX.void-void.d
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +/*
> > + * This should fail compilation due to accessing args[1] (not args[0]) for a
> > + * void function without any arguuments.
> > + */
> > +fbt:vmlinux:oops_enter:return
> > +{
> > + trace(args[0]);
> > + trace(args[1]);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
> > diff --git a/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d b/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> > new file mode 100644
> > index 00000000..516d733a
> > --- /dev/null
> > +++ b/test/unittest/fbtprovider/err.D_ARGS_IDX.void.d
> > @@ -0,0 +1,24 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +/*
> > + * This should fail compilation due to accessing args[1] (not args[0]) for a
> > + * void function with arguments.
> > + */
> > +fbt:vmlinux:exit_creds:return
> > +{
> > + trace(args[0]);
> > + trace(args[1]);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + exit(1);
> > +}
More information about the DTrace-devel
mailing list