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

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

Okay.  I posted a v2.  I also posted a v5 of "Deprecate EPID" since it 
experiences a minor side effect.

On 9/12/24 16:33, Kris Van Hees wrote:
> 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
>>>>>