Re: [PATCH v2] error: ERROR probe firing should not corrupt probe arguments

[Eugene Loh <eugene.loh@oracle.com>]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
Might be nice to have an explanation ("ease pressure on the BPF stack") 
why this is put in a map rather than on the stack, but not essential.

On 9/12/24 13:59, Kris Van Hees wrote:
> When an ERROR probe fires, it overwrites the first 6 probe arguments.
> If other clauses are then executed for the original probe, the probe
> arguments will have been corrupted.
>
> Save arg0 through arg5 prior to calling the ERROR probe, and restore
> them after the ERROR probe finishes.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   bpf/probe_error.c                             |  2 +
>   libdtrace/dt_cg.c                             |  4 +-
>   libdtrace/dt_dctx.h                           |  5 +-
>   .../error/tst.argv-corruption-by-error.d      | 48 +++++++++++++++++++
>   4 files changed, 55 insertions(+), 4 deletions(-)
>   create mode 100644 test/unittest/error/tst.argv-corruption-by-error.d
>
> diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> index c8ddcdfa..cad161fd 100644
> --- a/bpf/probe_error.c
> +++ b/bpf/probe_error.c
> @@ -27,6 +27,7 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>   {
>   	dt_mstate_t	*mst = dctx->mst;
>   
> +	__builtin_memcpy(mst->saved_argv, mst->argv, sizeof(mst->saved_argv));
>   	mst->argv[0] = 0;
>   	mst->argv[1] = mst->epid;
>   	mst->argv[2] = mst->clid;
> @@ -36,5 +37,6 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
>   
>   	dt_error(dctx);
>   
> +	__builtin_memcpy(mst->argv, mst->saved_argv, sizeof(mst->saved_argv));
>   	mst->fault = fault;
>   }
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 4fab8eed..166627fb 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -818,7 +818,7 @@ dt_cg_tramp_save_args(dt_pcb_t *pcb)
>   
>   	for (i = 0; i < ARRAY_SIZE(((dt_mstate_t *)0)->argv); i++) {
>   		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ARG(i)));
> -		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_SAVED_ARG(i), BPF_REG_0));
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ORIG_ARG(i), BPF_REG_0));
>   	}
>   }
>   
> @@ -832,7 +832,7 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
>   	int		i;
>   
>   	for (i = 0; i < ARRAY_SIZE(((dt_mstate_t *)0)->argv); i++) {
> -		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_SAVED_ARG(i)));
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(i)));
>   		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
>   	}
>   }
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index 1422ad24..633c529f 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -30,7 +30,8 @@ typedef struct dt_mstate {
>   	uint64_t	tstamp;		/* cached timestamp value */
>   	dt_pt_regs	regs;		/* CPU registers */
>   	uint64_t	argv[10];	/* Probe arguments */
> -	uint64_t	saved_argv[10];	/* Saved probe arguments */
> +	uint64_t	orig_argv[10];	/* Original (underlying) probe args */
> +	uint64_t	saved_argv[6];	/* Saved arguments */
>   } dt_mstate_t;
>   
>   #define DMST_EPID		offsetof(dt_mstate_t, epid)
> @@ -45,7 +46,7 @@ typedef struct dt_mstate {
>   #define DMST_TSTAMP		offsetof(dt_mstate_t, tstamp)
>   #define DMST_REGS		offsetof(dt_mstate_t, regs)
>   #define DMST_ARG(n)		offsetof(dt_mstate_t, argv[n])
> -#define DMST_SAVED_ARG(n)	offsetof(dt_mstate_t, saved_argv[n])
> +#define DMST_ORIG_ARG(n)	offsetof(dt_mstate_t, orig_argv[n])
>   
>   /*
>    * The DTrace context.
> diff --git a/test/unittest/error/tst.argv-corruption-by-error.d b/test/unittest/error/tst.argv-corruption-by-error.d
> new file mode 100644
> index 00000000..6fd1834e
> --- /dev/null
> +++ b/test/unittest/error/tst.argv-corruption-by-error.d
> @@ -0,0 +1,48 @@
> +/*
> + * 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: A fault that triggers the ERROR probe terminates the execution of
> + *            the current clause, while other clauses for the same probe should
> + *            still be executed.  This tests that the ERROR probe invocation
> + *            does not corrupt the arguments of the original probe.
> + *
> + * SECTION: dtrace Provider
> + */
> +
> +#pragma D option quiet
> +
> +syscall::write*:entry
> +{
> +	self->arg0 = arg0;
> +	self->arg1 = arg1;
> +	self->arg2 = arg2;
> +	self->arg3 = arg3;
> +	self->arg4 = arg4;
> +	self->arg5 = arg5;
> +
> +	printf("%d / %d / %d / %d / %d / %d\n",
> +	       arg0, arg1, arg2, arg3, arg4, arg5);
> +}
> +
> +syscall::write*:entry
> +{
> +	trace(*(int *)0);
> +}
> +
> +syscall::write*:entry,
> +ERROR {
> +	printf("%d / %d / %d / %d / %d / %d\n",
> +	       arg0, arg1, arg2, arg3, arg4, arg5);
> +}
> +
> +syscall::write*:entry
> +{
> +	exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
> +	     self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
> +		? 1 : 0);
> +}