Relocation processing for the ERROR program is done too early

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 <[email protected]>

Signed-off-by Kris Van Hees <[email protected]>

Reviewed-by Kris Van Hees <[email protected]>

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

libdtrace/dt_bpf.c +21 -19

index 70597d65..df427c86 100644
@@ -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;
 

test/unittest/builtinvar/tst.probe_dtrace.d new file +34 -0

new file mode 100644
index 00000000..d08067ad
@@ -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);
+}

test/unittest/builtinvar/tst.probe_dtrace.r new file +6 -0

new file mode 100644
index 00000000..e6cccc88
@@ -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

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?

Quoted 156 lines

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 <[email protected]>

Signed-off-by Kris Van Hees <[email protected]>

Reviewed-by Kris Van Hees <[email protected]>

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

libdtrace/dt_bpf.c +21 -19

index 70597d65..df427c86 100644
@@ -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;

test/unittest/builtinvar/tst.probe_dtrace.d new file +34 -0

new file mode 100644
index 00000000..d08067ad
@@ -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);
+}

test/unittest/builtinvar/tst.probe_dtrace.r new file +6 -0

new file mode 100644
index 00000000..e6cccc88
@@ -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