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

[Kris Van Hees <kris.van.hees@oracle.com>]

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