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

[Eugene Loh <eugene.loh@oracle.com>]

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

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

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