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

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

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.

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!

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
>