* [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes
@ 2024-08-02 16:35 Kris Van Hees
2024-08-02 17:45 ` Eugene Loh
0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2024-08-02 16:35 UTC (permalink / raw)
To: dtrace, dtrace-devel
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@oracle.com>
Signed-off-by: Kris Van Hees <kris.van.hees@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);
+}
--
2.45.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes
2024-08-02 16:35 [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes Kris Van Hees
@ 2024-08-02 17:45 ` Eugene Loh
2024-08-02 17:52 ` Kris Van Hees
0 siblings, 1 reply; 3+ messages in thread
From: Eugene Loh @ 2024-08-02 17:45 UTC (permalink / raw)
To: Kris Van Hees, dtrace, dtrace-devel
Not a real careful review, since I'm not that familiar with the BTF
stuff, but:
Reviewed-by: Eugene Loh <eugene.loh@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@oracle.com>
> Signed-off-by: Kris Van Hees <kris.van.hees@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);
> +}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes
2024-08-02 17:45 ` Eugene Loh
@ 2024-08-02 17:52 ` Kris Van Hees
0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2024-08-02 17:52 UTC (permalink / raw)
To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel
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@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@oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees@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);
> > +}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-02 17:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-02 16:35 [PATCH v3] bpf, fbt: do not try to get a return value for void function return pronbes Kris Van Hees
2024-08-02 17:45 ` Eugene Loh
2024-08-02 17:52 ` Kris Van Hees
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox