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

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

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