[DTrace-devel] [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes
Eugene Loh
eugene.loh at oracle.com
Fri Aug 2 17:45:46 UTC 2024
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?
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