[DTrace-devel] [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 at 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);
> > +}