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

Eugene Loh
2024-09-12T18:51:05Z

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);
> +}