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

Eugene Loh
2024-09-07T01:25:21Z

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