[PATCH] Relocation processing of STBSZ in ERROR's get_bvar() is too early

[PATCH] Relocation processing of STBSZ in ERROR's get_bvar() is too early

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

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.

Change dt_bpf_reloc_prog() -- which performs relocation processing for
BPF maps, necessarily after dt_bpf_gmap_create()! -- to check STBSZ
and set its value if necessary.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_bpf.c                          | 17 +++++++++++
 test/unittest/builtinvar/tst.probe_dtrace.d | 34 +++++++++++++++++++++
 test/unittest/builtinvar/tst.probe_dtrace.r |  6 ++++
 3 files changed, 57 insertions(+)
 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 0df1a9b9b..5cce747fe 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -1124,6 +1124,23 @@ dt_bpf_reloc_prog(dtrace_hdl_t *dtp, const dtrace_difo_t *dp)
 				text[ioff + 1].imm = 0;
 			} else if (rp->dofr_type == R_BPF_64_32)
 				text[ioff].imm = val;
+		} else if (idp->di_kind == DT_IDENT_SCALAR &&
+			   idp->di_id == DT_CONST_STBSZ) {
+			/*
+			 * There is one other case.  Although STBSZ is not a map
+			 * pointer, the string table size is set when the BPF maps
+			 * are created.  So we may have to set its value here.
+			 */
+			assert(strcmp(name, "STBSZ") == 0);  // FIXME:  extra check for development
+
+			/*
+			 * Worry only about the cases that have not yet
+			 * been handled.
+			 */
+			if (rp->dofr_type != R_BPF_64_64 && rp->dofr_data == 0) {
+				text[ioff].imm = dtp->dt_strmax;
+				text[ioff + 1].imm = 0;
+			}
 		}
 	}
 }
diff --git a/test/unittest/builtinvar/tst.probe_dtrace.d b/test/unittest/builtinvar/tst.probe_dtrace.d
new file mode 100644
index 000000000..d08067ad0
--- /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 000000000..e6cccc88d
--- /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.43.5

Re: [DTrace-devel] [PATCH] Relocation processing of STBSZ in ERROR's get_bvar() is too early

[Kris Van Hees]

I don't thibk this is the right solution.  There are other values that are
resolved as a relocation in dt_link_construct() and that could be found in
an ERROR clause and thus get resolved before the correct value is known.

Instead, I think we need to simply move the construction of the dt_error
program into dt_bpf_load_progs().  I should have done that when I split
up program construction and loading into two distinct phases.

I'll post an alternative patch in a moment...

On Fri, Sep 13, 2024 at 07:36:04PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> 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.
> 
> Change dt_bpf_reloc_prog() -- which performs relocation processing for
> BPF maps, necessarily after dt_bpf_gmap_create()! -- to check STBSZ
> and set its value if necessary.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>  libdtrace/dt_bpf.c                          | 17 +++++++++++
>  test/unittest/builtinvar/tst.probe_dtrace.d | 34 +++++++++++++++++++++
>  test/unittest/builtinvar/tst.probe_dtrace.r |  6 ++++
>  3 files changed, 57 insertions(+)
>  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 0df1a9b9b..5cce747fe 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1124,6 +1124,23 @@ dt_bpf_reloc_prog(dtrace_hdl_t *dtp, const dtrace_difo_t *dp)
>  				text[ioff + 1].imm = 0;
>  			} else if (rp->dofr_type == R_BPF_64_32)
>  				text[ioff].imm = val;
> +		} else if (idp->di_kind == DT_IDENT_SCALAR &&
> +			   idp->di_id == DT_CONST_STBSZ) {
> +			/*
> +			 * There is one other case.  Although STBSZ is not a map
> +			 * pointer, the string table size is set when the BPF maps
> +			 * are created.  So we may have to set its value here.
> +			 */
> +			assert(strcmp(name, "STBSZ") == 0);  // FIXME:  extra check for development
> +
> +			/*
> +			 * Worry only about the cases that have not yet
> +			 * been handled.
> +			 */
> +			if (rp->dofr_type != R_BPF_64_64 && rp->dofr_data == 0) {
> +				text[ioff].imm = dtp->dt_strmax;
> +				text[ioff + 1].imm = 0;
> +			}
>  		}
>  	}
>  }
> diff --git a/test/unittest/builtinvar/tst.probe_dtrace.d b/test/unittest/builtinvar/tst.probe_dtrace.d
> new file mode 100644
> index 000000000..d08067ad0
> --- /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 000000000..e6cccc88d
> --- /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.43.5
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

Re: [DTrace-devel] [PATCH] Relocation processing of STBSZ in ERROR's get_bvar() is too early

[Eugene Loh]

On 9/13/24 22:53, Kris Van Hees wrote:
> I don't thibk this is the right solution.  There are other values that are
> resolved as a relocation in dt_link_construct() and that could be found in
> an ERROR clause and thus get resolved before the correct value is known.
>
> Instead, I think we need to simply move the construction of the dt_error
> program into dt_bpf_load_progs().  I should have done that when I split
> up program construction and loading into two distinct phases.
>
> I'll post an alternative patch in a moment...

Thanks.  I confess I posted this patch impatiently, with the uneasy 
suspicion that the relocation was simply being performed at the wrong time.

> On Fri, Sep 13, 2024 at 07:36:04PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> 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.
>>
>> Change dt_bpf_reloc_prog() -- which performs relocation processing for
>> BPF maps, necessarily after dt_bpf_gmap_create()! -- to check STBSZ
>> and set its value if necessary.