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

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

[Kris Van Hees]

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.

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];
 
+	__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 */
 } 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
+ *            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);
+}
-- 
2.45.2

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

[Eugene Loh]

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

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

[Kris Van Hees]

On Tue, Sep 10, 2024 at 01:51:41AM -0400, Eugene Loh wrote:
> 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."

Thanks.

> > 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?

Ah, oops, meant to remove argv[] here.  Will do that.

> > +	__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.

No, because that makes the stack too large and then the BPF verifier rejects
the program.  I hope to rewrite the ERROR probe support in the future to use
a different mechanism, but that needs more design.

> >   } 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.

Sure.

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

In this case, dtrace serves as the trigger itself and given the arguments to
the write syscall, the ERROR probe arguments cannot be mistaken for those of
the write* syscalls.  So, this is certainly sufficient.  And since we compare
the arguments of the first clause with those of the third, we can clearly
detect validly whether the arguments for corrupted or not.

> > +{
> > +	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.

Not really - the sequence is pretty clear from the script.

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

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

[Eugene Loh]

On 9/10/24 02:20, Kris Van Hees wrote:

> On Tue, Sep 10, 2024 at 01:51:41AM -0400, Eugene Loh wrote:
>> On 9/10/24 00:33, Kris Van Hees wrote:
>>> diff --git 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.
> No, because that makes the stack too large and then the BPF verifier rejects
> the program.

Just to be clear, do you mean the test program will fail?  Or the test 
program passes, but you simply do not want to add to stack pressure?

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

[Kris Van Hees]

On Tue, Sep 10, 2024 at 11:20:28AM -0400, Eugene Loh wrote:
> On 9/10/24 02:20, Kris Van Hees wrote:
> 
> > On Tue, Sep 10, 2024 at 01:51:41AM -0400, Eugene Loh wrote:
> > > On 9/10/24 00:33, Kris Van Hees wrote:
> > > > diff --git 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.
> > No, because that makes the stack too large and then the BPF verifier rejects
> > the program.
> 
> Just to be clear, do you mean the test program will fail?  Or the test
> program passes, but you simply do not want to add to stack pressure?

No, the test program will not fail with argv[] in dt_probe_error(), but other
tests *do* fail if we store the arguments on the stack.

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

[Kris Van Hees]

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

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

[Eugene Loh]

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