[DTrace-devel] [PATCH 01/14] Have dt_cg_load_scalar report runtime error if it fails
Kris Van Hees
kris.van.hees at oracle.com
Thu May 4 03:42:37 UTC 2023
First off, this is a very welcomed patch!
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
... with small changes (to comment content) noted below
On Mon, May 01, 2023 at 11:47:09PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 17 +++++++++++++++--
> test/unittest/codegen/err.deref_0.d | 23 +++++++++++++++++++++++
> test/unittest/codegen/err.deref_0.r | 3 +++
> test/unittest/codegen/err.deref_0.r.p | 6 ++++++
> test/unittest/codegen/err.deref_1.d | 23 +++++++++++++++++++++++
> test/unittest/codegen/err.deref_1.r | 3 +++
> test/unittest/codegen/err.deref_1.r.p | 6 ++++++
> test/unittest/codegen/err.deref_i0.d | 24 ++++++++++++++++++++++++
> test/unittest/codegen/err.deref_i0.r | 3 +++
> test/unittest/codegen/err.deref_i0.r.p | 6 ++++++
> test/unittest/codegen/err.deref_i1.d | 24 ++++++++++++++++++++++++
> test/unittest/codegen/err.deref_i1.r | 3 +++
> test/unittest/codegen/err.deref_i1.r.p | 6 ++++++
> 13 files changed, 145 insertions(+), 2 deletions(-)
> create mode 100644 test/unittest/codegen/err.deref_0.d
> create mode 100644 test/unittest/codegen/err.deref_0.r
> create mode 100755 test/unittest/codegen/err.deref_0.r.p
> create mode 100644 test/unittest/codegen/err.deref_1.d
> create mode 100644 test/unittest/codegen/err.deref_1.r
> create mode 100755 test/unittest/codegen/err.deref_1.r.p
> create mode 100644 test/unittest/codegen/err.deref_i0.d
> create mode 100644 test/unittest/codegen/err.deref_i0.r
> create mode 100755 test/unittest/codegen/err.deref_i0.r.p
> create mode 100644 test/unittest/codegen/err.deref_i1.d
> create mode 100644 test/unittest/codegen/err.deref_i1.r
> create mode 100755 test/unittest/codegen/err.deref_i1.r.p
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 14ae21f7..483603ef 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2468,10 +2468,16 @@ dt_cg_promote(const dt_node_t *dnp, ssize_t size, dt_irlist_t *dlp,
> }
> }
>
> +/*
> + * Dereference a pointer to a scalar that is in potentially unsafe memory.
> + */
I think this is putting a bit too much "how it is used" in the comment rather
than juts describing what the function does.
How about simply: Load a scalar from an arbitrary address.
> static void
> dt_cg_load_scalar(dt_node_t *dnp, uint_t op, ssize_t size, dt_irlist_t *dlp,
> dt_regset_t *drp)
> {
> + uint_t Lokay = dt_irlist_label(dlp);
> +
> + /* copy the potentially unsafe memory into the D stack */
I am not sure there is much value in talking ab out unsafe memory, because
that could mean a multitude of things. And really, this function is used to
load scalars from *any* address that is not known to be a pointer to D managed
memory. So, most of the time, it wouldn't be considered unsafe (except by the
BPF verifier).
How about: Copy the scalar onto the stack.
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> @@ -2479,11 +2485,18 @@ dt_cg_load_scalar(dt_node_t *dnp, uint_t op, ssize_t size, dt_irlist_t *dlp,
> emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> - dt_regset_free(drp, BPF_REG_0);
> dt_regset_free_args(drp);
> +
> + /* check if we were successful */
Hoe about: Report a fault if the copy failed.
(and fix indentation of the comment)
> + emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, Lokay));
> + dt_regset_free(drp, BPF_REG_0);
> + dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, DT_ISREG, dnp->dn_reg);
> + emitl(dlp, Lokay,
> + BPF_NOP());
> +
> + /* load the copy of the data in "safe" memory (D stack) */
Similarly: Load the scalar from the stack.
> emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_SP));
> emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
> -
> dt_cg_promote(dnp, size, dlp, drp);
> }
>
> diff --git a/test/unittest/codegen/err.deref_0.d b/test/unittest/codegen/err.deref_0.d
> new file mode 100644
> index 00000000..0149513b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_0.d
> @@ -0,0 +1,23 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(*((char*)0));
> +}
> +
> +BEGIN
> +{
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/err.deref_0.r b/test/unittest/codegen/err.deref_0.r
> new file mode 100644
> index 00000000..07c1dc52
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_0.r
> @@ -0,0 +1,3 @@
> +
> +-- @@stderr --
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address (0) in action #1 at BPF pc NNN
> diff --git a/test/unittest/codegen/err.deref_0.r.p b/test/unittest/codegen/err.deref_0.r.p
> new file mode 100755
> index 00000000..68ebc99b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_0.r.p
> @@ -0,0 +1,6 @@
> +#!/usr/bin/sed -f
> +
> +# runtest.sh looks for "0x" to filter out pointer values.
> +# Strip the 0x so that the illegal address will not be filtered out;
> +# we want the address to be checked.
> +s/0x//
> diff --git a/test/unittest/codegen/err.deref_1.d b/test/unittest/codegen/err.deref_1.d
> new file mode 100644
> index 00000000..f0d9719b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_1.d
> @@ -0,0 +1,23 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(*((char*)1));
> +}
> +
> +BEGIN
> +{
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/err.deref_1.r b/test/unittest/codegen/err.deref_1.r
> new file mode 100644
> index 00000000..a2ca8ac4
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_1.r
> @@ -0,0 +1,3 @@
> +
> +-- @@stderr --
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address (1) in action #1 at BPF pc NNN
> diff --git a/test/unittest/codegen/err.deref_1.r.p b/test/unittest/codegen/err.deref_1.r.p
> new file mode 100755
> index 00000000..68ebc99b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_1.r.p
> @@ -0,0 +1,6 @@
> +#!/usr/bin/sed -f
> +
> +# runtest.sh looks for "0x" to filter out pointer values.
> +# Strip the 0x so that the illegal address will not be filtered out;
> +# we want the address to be checked.
> +s/0x//
> diff --git a/test/unittest/codegen/err.deref_i0.d b/test/unittest/codegen/err.deref_i0.d
> new file mode 100644
> index 00000000..228c412c
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i0.d
> @@ -0,0 +1,24 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + i = 0;
> + trace(*((char*)i));
> +}
> +
> +BEGIN
> +{
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/err.deref_i0.r b/test/unittest/codegen/err.deref_i0.r
> new file mode 100644
> index 00000000..07c1dc52
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i0.r
> @@ -0,0 +1,3 @@
> +
> +-- @@stderr --
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address (0) in action #1 at BPF pc NNN
> diff --git a/test/unittest/codegen/err.deref_i0.r.p b/test/unittest/codegen/err.deref_i0.r.p
> new file mode 100755
> index 00000000..68ebc99b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i0.r.p
> @@ -0,0 +1,6 @@
> +#!/usr/bin/sed -f
> +
> +# runtest.sh looks for "0x" to filter out pointer values.
> +# Strip the 0x so that the illegal address will not be filtered out;
> +# we want the address to be checked.
> +s/0x//
> diff --git a/test/unittest/codegen/err.deref_i1.d b/test/unittest/codegen/err.deref_i1.d
> new file mode 100644
> index 00000000..b47ba198
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i1.d
> @@ -0,0 +1,24 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + i = 1;
> + trace(*((char*)i));
> +}
> +
> +BEGIN
> +{
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/err.deref_i1.r b/test/unittest/codegen/err.deref_i1.r
> new file mode 100644
> index 00000000..a2ca8ac4
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i1.r
> @@ -0,0 +1,3 @@
> +
> +-- @@stderr --
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address (1) in action #1 at BPF pc NNN
> diff --git a/test/unittest/codegen/err.deref_i1.r.p b/test/unittest/codegen/err.deref_i1.r.p
> new file mode 100755
> index 00000000..68ebc99b
> --- /dev/null
> +++ b/test/unittest/codegen/err.deref_i1.r.p
> @@ -0,0 +1,6 @@
> +#!/usr/bin/sed -f
> +
> +# runtest.sh looks for "0x" to filter out pointer values.
> +# Strip the 0x so that the illegal address will not be filtered out;
> +# we want the address to be checked.
> +s/0x//
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list