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

Eugene Loh
2024-09-10T05:51:53Z

On 9/10/24 00:33, Kris Van Hees wrote:

> When an ERROR probe fires due to a fault happening while processing a
> probe firing, it was corrupting the first 6 probe arguments of the
> probe causing the fault because they were being overwritten. But since
> a fault only aborts the execution of the clause it occurs in, those
> original probe arguments might still be needed for other clauses that
> are executed for the original probe.

"due to a fault... probe firing" makes the text cumbersome;  readers
already know why an ERROR probe fires.

How about replacing the paragraph with, "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 | 3 ++
> libdtrace/dt_dctx.h | 1 +
> .../error/tst.argv-corruption-by-error.d | 48 +++++++++++++++++++
> 3 files changed, 52 insertions(+)
> 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..8d631704 100644
> --- a/bpf/probe_error.c
> +++ b/bpf/probe_error.c
> @@ -26,7 +26,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> uint64_t illval)
> {
> dt_mstate_t *mst = dctx->mst;
> + uint64_t argv[6];

There is a compiler warning that argv[] is not used.  Did you mean to
use argv[] instead of msg->error_argv?

> + __builtin_memcpy(mst->error_argv, mst->argv, sizeof(mst->error_argv));
> mst->argv[0] = 0;
> mst->argv[1] = mst->epid;
> mst->argv[2] = mst->clid;
> @@ -36,5 +38,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->error_argv, sizeof(mst->error_argv));
> mst->fault = fault;
> }
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index 1422ad24..9f33f1fb 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -31,6 +31,7 @@ typedef struct dt_mstate {
> dt_pt_regs regs; /* CPU registers */
> uint64_t argv[10]; /* Probe arguments */
> uint64_t saved_argv[10]; /* Saved probe arguments */
> + uint64_t error_argv[6]; /* ERROR probe saved arguments */

I don't think such a thing is needed in mstate.  It suffices to use
local storage in dt_probe_error(), which is probably what you really
meant to do.  Plus, calling it error_argv[] is misleading since these
are the args that you are *NOT* using for the ERROR probe.

> } dt_mstate_t;
>
> #define DMST_EPID offsetof(dt_mstate_t, epid)
> 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

"While" is a confusing word since it could mean "during" (which was the
sense I assumed when I read this).  How about s/while/but/ or something.

> + * 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

This ought to work, but strictly speaking I suppose there should be a
trigger.  In fact, it might be nice to use a trigger whose arguments are
known.  How about a test that looks like test/unittest/pid/tst.args1.d?
This could also illustrate (without the fix) that only arg0-arg5 are
afflicted.

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

Maybe it'd be nice to have the printf()s also emit labels so that if
someone needs to look at the output it's clearer what they're seeing.

> +
> +syscall::write*:entry
> +{
> + exit(self->arg0 != arg0 || self->arg1 != arg1 || self->arg2 != arg2 ||
> + self->arg3 != arg3 || self->arg4 != arg4 || self->arg5 != arg5
> + ? 1 : 0);
> +}