* [PATCH] Relocation processing for the ERROR program is done too early
@ 2024-09-14 2:58 Kris Van Hees
2024-09-16 22:58 ` Eugene Loh
0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2024-09-14 2:58 UTC (permalink / raw)
To: eugene.loh, dtrace, dtrace-devel
When we call dtrace_go(), we do something like this:
dt_bpf_make_progs()
dt_program_construct() // just for ERROR
dt_link()
dt_link_construct()
dt_bpf_gmap_create()
dt_bpf_load_progs() // other
dt_link()
dt_link_construct()
In dt_link_construct() we dive down and find dt_get_bvar(). One of the
relocations is to supply the value of STBSZ. The first dt_link() is for
ERROR, while the subsequent calls in dt_bpf_load_progs() are for other
clauses -- that is, two separate versions of dt_get_bvar() are used.
Meanwhile, the value of STBSZ is not set until dt_bpf_gmap_create().
This means that the ERROR copy of dt_get_bvar() does not have STBSZ set
properly. This means that if ERROR accesses probeprov or probename,
dt_get_bvar() returns the beginning of the string table, which is a NUL
terminator. Some other relocation values have similar issues.
Move the ERROR program construction to dt_bpf_load_progs().
Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
---
libdtrace/dt_bpf.c | 40 +++++++++++----------
test/unittest/builtinvar/tst.probe_dtrace.d | 34 ++++++++++++++++++
test/unittest/builtinvar/tst.probe_dtrace.r | 6 ++++
3 files changed, 61 insertions(+), 19 deletions(-)
create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.d
create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.r
diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
index 70597d65..df427c86 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -1212,30 +1212,14 @@ int
dt_bpf_make_progs(dtrace_hdl_t *dtp, uint_t cflags)
{
dt_probe_t *prp;
- dtrace_difo_t *dp;
- dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
-
- assert(idp != NULL);
-
- /*
- * First construct the ERROR probe program (to be included in probe
- * programs that may trigger a fault).
- *
- * After constructing the program, we need to patch up any calls to
- * dt_error because DTrace cannot handle faults in ERROR itself.
- */
- dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
- if (dp == NULL)
- return -1;
-
- idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
- dt_bpf_reloc_error_prog(dtp, dp);
/*
* Now construct all the other programs.
*/
for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
prp = dt_list_next(prp)) {
+ dtrace_difo_t *dp;
+
/* Already done. */
if (prp == dtp->dt_error)
continue;
@@ -1263,7 +1247,25 @@ int
dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
{
dt_probe_t *prp;
+ dtrace_difo_t *dp;
dtrace_optval_t dest_ok = DTRACEOPT_UNSET;
+ dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
+
+ assert(idp != NULL);
+
+ /*
+ * First construct the ERROR probe program (to be linked in probe
+ * programs that may trigger a fault).
+ *
+ * After constructing the program, we need to patch up any calls to
+ * dt_error because DTrace cannot handle faults in ERROR itself (yet).
+ */
+ dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
+ if (dp == NULL)
+ return -1;
+
+ idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
+ dt_bpf_reloc_error_prog(dtp, dp);
/*
* Determine whether we can allow destructive actions.
@@ -1272,10 +1274,10 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
prp = dt_list_next(prp)) {
- dtrace_difo_t *dp = prp->difo;
int fd;
int rc = -1;
+ dp = prp->difo;
if (dp == NULL)
continue;
diff --git a/test/unittest/builtinvar/tst.probe_dtrace.d b/test/unittest/builtinvar/tst.probe_dtrace.d
new file mode 100644
index 00000000..d08067ad
--- /dev/null
+++ b/test/unittest/builtinvar/tst.probe_dtrace.d
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+/*
+ * ASSERTION:
+ * Built-in variables listing provider, module, function, and name
+ * are correct from dtrace-provider probes.
+ *
+ * SECTION: Variables/Built-in Variables
+ */
+
+#pragma D option quiet
+
+/* Check build-in variables from the dtrace-provider probes. */
+BEGIN,ERROR,END
+{
+ printf("%s:%s:%s:%s\n", probeprov, probemod, probefunc, probename);
+}
+
+/* Cause the ERROR probe to fire. */
+BEGIN
+{
+ *((int*)0);
+}
+
+/* Cause the END probe to fire. */
+BEGIN,ERROR
+{
+ exit(0);
+}
diff --git a/test/unittest/builtinvar/tst.probe_dtrace.r b/test/unittest/builtinvar/tst.probe_dtrace.r
new file mode 100644
index 00000000..e6cccc88
--- /dev/null
+++ b/test/unittest/builtinvar/tst.probe_dtrace.r
@@ -0,0 +1,6 @@
+dtrace:::BEGIN
+dtrace:::ERROR
+dtrace:::END
+
+-- @@stderr --
+dtrace: error in dt_clause_3 for probe ID 1 (dtrace:::BEGIN): invalid address ({ptr}) at BPF pc NNN
--
2.45.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Relocation processing for the ERROR program is done too early
2024-09-14 2:58 [PATCH] Relocation processing for the ERROR program is done too early Kris Van Hees
@ 2024-09-16 22:58 ` Eugene Loh
2024-09-18 9:02 ` Kris Van Hees
0 siblings, 1 reply; 3+ messages in thread
From: Eugene Loh @ 2024-09-16 22:58 UTC (permalink / raw)
To: dtrace, dtrace-devel
Thanks. I see your R-b. So, no further review is needed? If this
patch has broader scope than the initial patch, should further checks be
added to the test -- e.g., to demonstrate that other issues are fixed as
well?
On 9/13/24 22:58, Kris Van Hees wrote:
> When we call dtrace_go(), we do something like this:
>
> dt_bpf_make_progs()
> dt_program_construct() // just for ERROR
> dt_link()
> dt_link_construct()
> dt_bpf_gmap_create()
> dt_bpf_load_progs() // other
> dt_link()
> dt_link_construct()
>
> In dt_link_construct() we dive down and find dt_get_bvar(). One of the
> relocations is to supply the value of STBSZ. The first dt_link() is for
> ERROR, while the subsequent calls in dt_bpf_load_progs() are for other
> clauses -- that is, two separate versions of dt_get_bvar() are used.
> Meanwhile, the value of STBSZ is not set until dt_bpf_gmap_create().
> This means that the ERROR copy of dt_get_bvar() does not have STBSZ set
> properly. This means that if ERROR accesses probeprov or probename,
> dt_get_bvar() returns the beginning of the string table, which is a NUL
> terminator. Some other relocation values have similar issues.
>
> Move the ERROR program construction to dt_bpf_load_progs().
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> libdtrace/dt_bpf.c | 40 +++++++++++----------
> test/unittest/builtinvar/tst.probe_dtrace.d | 34 ++++++++++++++++++
> test/unittest/builtinvar/tst.probe_dtrace.r | 6 ++++
> 3 files changed, 61 insertions(+), 19 deletions(-)
> create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.d
> create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.r
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 70597d65..df427c86 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1212,30 +1212,14 @@ int
> dt_bpf_make_progs(dtrace_hdl_t *dtp, uint_t cflags)
> {
> dt_probe_t *prp;
> - dtrace_difo_t *dp;
> - dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
> -
> - assert(idp != NULL);
> -
> - /*
> - * First construct the ERROR probe program (to be included in probe
> - * programs that may trigger a fault).
> - *
> - * After constructing the program, we need to patch up any calls to
> - * dt_error because DTrace cannot handle faults in ERROR itself.
> - */
> - dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
> - if (dp == NULL)
> - return -1;
> -
> - idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
> - dt_bpf_reloc_error_prog(dtp, dp);
>
> /*
> * Now construct all the other programs.
> */
> for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
> prp = dt_list_next(prp)) {
> + dtrace_difo_t *dp;
> +
> /* Already done. */
> if (prp == dtp->dt_error)
> continue;
> @@ -1263,7 +1247,25 @@ int
> dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> {
> dt_probe_t *prp;
> + dtrace_difo_t *dp;
> dtrace_optval_t dest_ok = DTRACEOPT_UNSET;
> + dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
> +
> + assert(idp != NULL);
> +
> + /*
> + * First construct the ERROR probe program (to be linked in probe
> + * programs that may trigger a fault).
> + *
> + * After constructing the program, we need to patch up any calls to
> + * dt_error because DTrace cannot handle faults in ERROR itself (yet).
> + */
> + dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
> + if (dp == NULL)
> + return -1;
> +
> + idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
> + dt_bpf_reloc_error_prog(dtp, dp);
>
> /*
> * Determine whether we can allow destructive actions.
> @@ -1272,10 +1274,10 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
>
> for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
> prp = dt_list_next(prp)) {
> - dtrace_difo_t *dp = prp->difo;
> int fd;
> int rc = -1;
>
> + dp = prp->difo;
> if (dp == NULL)
> continue;
>
> diff --git a/test/unittest/builtinvar/tst.probe_dtrace.d b/test/unittest/builtinvar/tst.probe_dtrace.d
> new file mode 100644
> index 00000000..d08067ad
> --- /dev/null
> +++ b/test/unittest/builtinvar/tst.probe_dtrace.d
> @@ -0,0 +1,34 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION:
> + * Built-in variables listing provider, module, function, and name
> + * are correct from dtrace-provider probes.
> + *
> + * SECTION: Variables/Built-in Variables
> + */
> +
> +#pragma D option quiet
> +
> +/* Check build-in variables from the dtrace-provider probes. */
> +BEGIN,ERROR,END
> +{
> + printf("%s:%s:%s:%s\n", probeprov, probemod, probefunc, probename);
> +}
> +
> +/* Cause the ERROR probe to fire. */
> +BEGIN
> +{
> + *((int*)0);
> +}
> +
> +/* Cause the END probe to fire. */
> +BEGIN,ERROR
> +{
> + exit(0);
> +}
> diff --git a/test/unittest/builtinvar/tst.probe_dtrace.r b/test/unittest/builtinvar/tst.probe_dtrace.r
> new file mode 100644
> index 00000000..e6cccc88
> --- /dev/null
> +++ b/test/unittest/builtinvar/tst.probe_dtrace.r
> @@ -0,0 +1,6 @@
> +dtrace:::BEGIN
> +dtrace:::ERROR
> +dtrace:::END
> +
> +-- @@stderr --
> +dtrace: error in dt_clause_3 for probe ID 1 (dtrace:::BEGIN): invalid address ({ptr}) at BPF pc NNN
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Relocation processing for the ERROR program is done too early
2024-09-16 22:58 ` Eugene Loh
@ 2024-09-18 9:02 ` Kris Van Hees
0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2024-09-18 9:02 UTC (permalink / raw)
To: Eugene Loh; +Cc: dtrace, dtrace-devel
On Mon, Sep 16, 2024 at 06:58:01PM -0400, Eugene Loh wrote:
> Thanks. I see your R-b. So, no further review is needed? If this patch
> has broader scope than the initial patch, should further checks be added to
> the test -- e.g., to demonstrate that other issues are fixed as well?
Yes and no. You found the first known instance of this issue, and while I
indicated that there are other cases, testing them all would be tricky (and
may change over time). I think that the test you have is sufficient for all
cases because the fix is not specific to the test.
> On 9/13/24 22:58, Kris Van Hees wrote:
> > When we call dtrace_go(), we do something like this:
> >
> > dt_bpf_make_progs()
> > dt_program_construct() // just for ERROR
> > dt_link()
> > dt_link_construct()
> > dt_bpf_gmap_create()
> > dt_bpf_load_progs() // other
> > dt_link()
> > dt_link_construct()
> >
> > In dt_link_construct() we dive down and find dt_get_bvar(). One of the
> > relocations is to supply the value of STBSZ. The first dt_link() is for
> > ERROR, while the subsequent calls in dt_bpf_load_progs() are for other
> > clauses -- that is, two separate versions of dt_get_bvar() are used.
> > Meanwhile, the value of STBSZ is not set until dt_bpf_gmap_create().
> > This means that the ERROR copy of dt_get_bvar() does not have STBSZ set
> > properly. This means that if ERROR accesses probeprov or probename,
> > dt_get_bvar() returns the beginning of the string table, which is a NUL
> > terminator. Some other relocation values have similar issues.
> >
> > Move the ERROR program construction to dt_bpf_load_progs().
> >
> > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> > libdtrace/dt_bpf.c | 40 +++++++++++----------
> > test/unittest/builtinvar/tst.probe_dtrace.d | 34 ++++++++++++++++++
> > test/unittest/builtinvar/tst.probe_dtrace.r | 6 ++++
> > 3 files changed, 61 insertions(+), 19 deletions(-)
> > create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.d
> > create mode 100644 test/unittest/builtinvar/tst.probe_dtrace.r
> >
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index 70597d65..df427c86 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -1212,30 +1212,14 @@ int
> > dt_bpf_make_progs(dtrace_hdl_t *dtp, uint_t cflags)
> > {
> > dt_probe_t *prp;
> > - dtrace_difo_t *dp;
> > - dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
> > -
> > - assert(idp != NULL);
> > -
> > - /*
> > - * First construct the ERROR probe program (to be included in probe
> > - * programs that may trigger a fault).
> > - *
> > - * After constructing the program, we need to patch up any calls to
> > - * dt_error because DTrace cannot handle faults in ERROR itself.
> > - */
> > - dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
> > - if (dp == NULL)
> > - return -1;
> > -
> > - idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
> > - dt_bpf_reloc_error_prog(dtp, dp);
> > /*
> > * Now construct all the other programs.
> > */
> > for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
> > prp = dt_list_next(prp)) {
> > + dtrace_difo_t *dp;
> > +
> > /* Already done. */
> > if (prp == dtp->dt_error)
> > continue;
> > @@ -1263,7 +1247,25 @@ int
> > dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> > {
> > dt_probe_t *prp;
> > + dtrace_difo_t *dp;
> > dtrace_optval_t dest_ok = DTRACEOPT_UNSET;
> > + dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_error");
> > +
> > + assert(idp != NULL);
> > +
> > + /*
> > + * First construct the ERROR probe program (to be linked in probe
> > + * programs that may trigger a fault).
> > + *
> > + * After constructing the program, we need to patch up any calls to
> > + * dt_error because DTrace cannot handle faults in ERROR itself (yet).
> > + */
> > + dp = dt_program_construct(dtp, dtp->dt_error, cflags, idp);
> > + if (dp == NULL)
> > + return -1;
> > +
> > + idp->di_flags |= DT_IDFLG_CGREG; /* mark it as inline-ready */
> > + dt_bpf_reloc_error_prog(dtp, dp);
> > /*
> > * Determine whether we can allow destructive actions.
> > @@ -1272,10 +1274,10 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> > for (prp = dt_list_next(&dtp->dt_enablings); prp != NULL;
> > prp = dt_list_next(prp)) {
> > - dtrace_difo_t *dp = prp->difo;
> > int fd;
> > int rc = -1;
> > + dp = prp->difo;
> > if (dp == NULL)
> > continue;
> > diff --git a/test/unittest/builtinvar/tst.probe_dtrace.d b/test/unittest/builtinvar/tst.probe_dtrace.d
> > new file mode 100644
> > index 00000000..d08067ad
> > --- /dev/null
> > +++ b/test/unittest/builtinvar/tst.probe_dtrace.d
> > @@ -0,0 +1,34 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +/*
> > + * ASSERTION:
> > + * Built-in variables listing provider, module, function, and name
> > + * are correct from dtrace-provider probes.
> > + *
> > + * SECTION: Variables/Built-in Variables
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +/* Check build-in variables from the dtrace-provider probes. */
> > +BEGIN,ERROR,END
> > +{
> > + printf("%s:%s:%s:%s\n", probeprov, probemod, probefunc, probename);
> > +}
> > +
> > +/* Cause the ERROR probe to fire. */
> > +BEGIN
> > +{
> > + *((int*)0);
> > +}
> > +
> > +/* Cause the END probe to fire. */
> > +BEGIN,ERROR
> > +{
> > + exit(0);
> > +}
> > diff --git a/test/unittest/builtinvar/tst.probe_dtrace.r b/test/unittest/builtinvar/tst.probe_dtrace.r
> > new file mode 100644
> > index 00000000..e6cccc88
> > --- /dev/null
> > +++ b/test/unittest/builtinvar/tst.probe_dtrace.r
> > @@ -0,0 +1,6 @@
> > +dtrace:::BEGIN
> > +dtrace:::ERROR
> > +dtrace:::END
> > +
> > +-- @@stderr --
> > +dtrace: error in dt_clause_3 for probe ID 1 (dtrace:::BEGIN): invalid address ({ptr}) at BPF pc NNN
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-09-18 9:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-14 2:58 [PATCH] Relocation processing for the ERROR program is done too early Kris Van Hees
2024-09-16 22:58 ` Eugene Loh
2024-09-18 9:02 ` 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