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

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

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