Re: [PATCH 05/22] Set the ERROR PRID in BPF code

[Kris Van Hees <kris.van.hees@oracle.com>]

In addition, I think that the test should compare the id reported by the
ERROR probe against the id given in dtrace -ln dtrace:::ERROR output.  Yes,
it will be 3 (and I think putting the following three defines in dt_dctx.h
would be helpful with that), but by doing the comparison against the dtrace -l
output avoids having to encode a constant value in the expected output (and
we cannot easily access the defines from the test).

/* Static probe IDs for the dtrace provider. */
#define DTRACE_BEGIN_ID		1
#define DTRACE_END_ID		2
#define DTRACE_ERROR_ID		3

On Fri, Sep 06, 2024 at 10:03:38PM -0400, Kris Van Hees wrote:
> On Fri, Sep 06, 2024 at 09:25:12PM -0400, Eugene Loh wrote:
> > On 9/6/24 20:20, Kris Van Hees wrote:
> > 
> > > On Thu, Aug 29, 2024 at 01:22:02AM -0400, eugene.loh@oracle.com wrote:
> > > > From: Eugene Loh <eugene.loh@oracle.com>
> > > > 
> > > > We use the fact that the ERROR PRID is always 3.
> > > Where do we use it?  We certainly should never override the probe id that is
> > > assigned by dtrace when providers provide probes.  Will it always be 3?  Almost
> > > certainly, yes.  But the right way to do this is to actually make the ERROR
> > > PRID available as a symbol that can be resolved at program load time, so that
> > > the dt_probe_error function can get to its value.
> > 
> > I'm confused?  I thought I did this and you suggested simply hardwiring the
> > value 3. https://oss.oracle.com/pipermail/dtrace-devel/2024-July/004989.html
> 
> True, though I didn't mean to just hardwire the value 3.  At a minimum, I think
> it ought to be a define to set the value of the BEGIN, END, and ERROR probe,
> so that a symbolic name can be used in code.  And that probably would mean that
> it is best to add a check in the dtrace provider to ensure that those are
> indeed the probe ids assigned to BEGIN/END/ERROR (and if not, something is very
> wrong).
> 
> > > Anyway, the need to restore the mst->prid value in this patch also highlights
> > > that there is a much bigger problem...  A fault in one clause contaminates the
> > > first 6 arguments of the probe, and since a fault only interrupts the execution
> > > of the current clause, following clauses will no longer see the correct values
> > > of the first 6 arguments but rather the ones that dt_probe_error() stored in
> > > mst->argv[0 .. 5].  That is a bug for sure!
> > 
> > Good point, but... a separate issue/patch?
> 
> Yes, already written :)
> 
> > > Here is a test that demonstrates this issue:
> > > 
> > > #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);
> > > }
> > > 
> > > So, we need the ERROR prid value exposed as an external symbol so that it can
> > > be used in dt_probe_error(), and a patch that fixes the contamination of the
> > > arg values.
> > > 
> > > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > > ---
> > > >   bpf/probe_error.c                         |  3 +++
> > > >   test/unittest/builtinvar/tst.id_ERROR.d   | 32 +++++++++++++++++++++++
> > > >   test/unittest/builtinvar/tst.id_ERROR.r   |  3 +++
> > > >   test/unittest/builtinvar/tst.id_ERROR.r.p |  4 +++
> > > >   4 files changed, 42 insertions(+)
> > > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.d
> > > >   create mode 100644 test/unittest/builtinvar/tst.id_ERROR.r
> > > >   create mode 100755 test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > 
> > > > diff --git a/bpf/probe_error.c b/bpf/probe_error.c
> > > > index c8ddcdfa..ee1a1793 100644
> > > > --- a/bpf/probe_error.c
> > > > +++ b/bpf/probe_error.c
> > > > @@ -26,6 +26,7 @@ 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;
> > > > +	int		oldprid = mst->prid;
> > > >   	mst->argv[0] = 0;
> > > >   	mst->argv[1] = mst->epid;
> > > > @@ -34,7 +35,9 @@ noinline void dt_probe_error(const dt_dctx_t *dctx, uint64_t pc, uint64_t fault,
> > > >   	mst->argv[4] = fault;
> > > >   	mst->argv[5] = illval;
> > > > +	mst->prid = 3;
> > > >   	dt_error(dctx);
> > > > +	mst->prid = oldprid;
> > > >   	mst->fault = fault;
> > > >   }
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.d b/test/unittest/builtinvar/tst.id_ERROR.d
> > > > new file mode 100644
> > > > index 00000000..59021c60
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.d
> > > > @@ -0,0 +1,32 @@
> > > > +/*
> > > > + * 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:
> > > > + * The id in the ERROR probe is 3.
> > > > + *
> > > > + * SECTION: Variables/Built-in Variables
> > > > + */
> > > > +
> > > > +#pragma D option quiet
> > > > +
> > > > +tick-1s
> > > > +{
> > > > +	/* trigger the ERROR probe */
> > > > +	trace(*((int*)0));
> > > > +}
> > > > +
> > > > +tick-2s
> > > > +{
> > > > +	exit(1);
> > > > +}
> > > > +
> > > > +ERROR
> > > > +{
> > > > +	printf("id of the ERROR probe = %d\n", id);
> > > > +	exit(0);
> > > > +}
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r b/test/unittest/builtinvar/tst.id_ERROR.r
> > > > new file mode 100644
> > > > index 00000000..95974abe
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r
> > > > @@ -0,0 +1,3 @@
> > > > +id of the ERROR probe = 3
> > > > +
> > > > +-- @@stderr --
> > > > diff --git a/test/unittest/builtinvar/tst.id_ERROR.r.p b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > new file mode 100755
> > > > index 00000000..884b43f4
> > > > --- /dev/null
> > > > +++ b/test/unittest/builtinvar/tst.id_ERROR.r.p
> > > > @@ -0,0 +1,4 @@
> > > > +#!/usr/bin/gawk -f
> > > > +
> > > > +# Drop the line with run-dependent PRID for profile probe.
> > > > +!/error on enabled probe ID/ { print }
> > > > -- 
> > > > 2.43.5
> > > >