[DTrace-devel] [PATCH] Relocation processing for the ERROR program is done too early

[Eugene Loh]

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 at oracle.com>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> Reviewed-by: Kris Van Hees <kris.van.hees at 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