[DTrace-devel] [PATCH 10/12] Add support for copyin() subroutine
Eugene Loh
eugene.loh at oracle.com
Thu Jul 21 18:08:45 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Plus a few questions below, basically of the "I'm curious" flavor.
On 7/13/22 12:17, Kris Van Hees via DTrace-devel wrote:
> The copyin() subroutine makes use of the alloca() subroutine to allocate
> the needed scratch space to copy data to.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index be1db4b1..9a4a80d0 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -4131,17 +4131,15 @@ dt_cg_subr_speculation(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> }
>
> static void
> -dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +dt_cg_subr_alloca_impl(dt_node_t *dnp, dt_node_t *size, dt_irlist_t *dlp,
> + dt_regset_t *drp)
> {
> - dt_node_t *size = dnp->dn_args;
> uint_t lbl_ok = dt_irlist_label(dlp);
> uint_t lbl_err = dt_irlist_label(dlp);
> int opt_scratchsize = yypcb->pcb_hdl->dt_options[DTRACEOPT_SCRATCHSIZE];
> int mst, next;
>
> - TRACE_REGSET(" subr-alloca:Begin");
> -
> - dt_cg_node(size, dlp, drp);
> + TRACE_REGSET(" subr-alloca-impl:Begin");
Is there a policy on how much to indent nested TRACE_REGSET(), or just
whatever anyone feels like?
How about allocating dnp->dn_reg inside of dt_cg_subr_alloca_impl()
rather than have each caller do it?
>
> /*
> * Compile-error out if the size is too large even absent other
> @@ -4159,8 +4157,7 @@ dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> (unsigned long) size->dn_value,
> (unsigned long) opt_scratchsize - 8);
>
> - if (((dnp->dn_reg = dt_regset_alloc(drp)) == -1) ||
> - ((mst = dt_regset_alloc(drp)) == -1) ||
> + if (((mst = dt_regset_alloc(drp)) == -1) ||
> ((next = dt_regset_alloc(drp)) == -1))
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> @@ -4188,6 +4185,24 @@ dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>
> dt_regset_free(drp, mst);
> dt_regset_free(drp, next);
> +
> + TRACE_REGSET(" subr-alloca-impl:End ");
> +}
> +
> +static void
> +dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> + dt_node_t *size = dnp->dn_args;
> +
> + TRACE_REGSET(" subr-alloca:Begin");
> +
> + dt_cg_node(size, dlp, drp);
> +
> + if ((dnp->dn_reg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + dt_cg_subr_alloca_impl(dnp, size, dlp, drp);
> +
> dt_regset_free(drp, size->dn_reg);
>
> TRACE_REGSET(" subr-alloca:End ");
> @@ -4259,6 +4274,57 @@ dt_cg_subr_bcopy(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> TRACE_REGSET(" subr-bcopy:End ");
> }
>
> +static void
> +dt_cg_subr_copyin(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> + dtrace_hdl_t *dtp = yypcb->pcb_hdl;
> + dt_node_t *src = dnp->dn_args;
> + dt_node_t *size = src->dn_list;
> + int maxsize = dtp->dt_options[DTRACEOPT_SCRATCHSIZE] - 8;
> + uint_t lbl_ok = dt_irlist_label(dlp);
> + uint_t lbl_badsize = dt_irlist_label(dlp);
> +
> + TRACE_REGSET(" subr-copyin:Begin");
> +
> + /* Validate the size for the copy operation. */
> + dt_cg_node(size, dlp, drp);
> +
> + emit(dlp, BPF_BRANCH_IMM(BPF_JSLT, size->dn_reg, 0, lbl_badsize));
> + emit(dlp, BPF_BRANCH_IMM(BPF_JGT, size->dn_reg, maxsize, lbl_badsize));
> +
> + if ((dnp->dn_reg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + /* Allocate scratch space. */
> + dt_cg_subr_alloca_impl(dnp, size, dlp, drp);
> + dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg, dnp->dn_reg);
> +
> + dt_cg_node(src, dlp, drp);
> +
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, dnp->dn_reg));
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, size->dn_reg));
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, src->dn_reg));
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_user]));
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_ok));
> + dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, DT_ISREG, src->dn_reg);
> + emitl(dlp, lbl_badsize,
> + BPF_NOP());
> + dt_cg_probe_error(yypcb, DTRACEFLT_BADSIZE, DT_ISREG, size->dn_reg);
> + emitl(dlp, lbl_ok,
> + BPF_NOP());
> + dt_regset_free(drp, BPF_REG_0);
> +
> + dt_regset_free(drp, src->dn_reg);
> + dt_regset_free(drp, size->dn_reg);
> +
> + TRACE_REGSET(" subr-copyin:End ");
> +}
> +
> static void
> dt_cg_subr_strchr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
> @@ -4730,7 +4796,7 @@ static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
> [DIF_SUBR_RW_READ_HELD] = &dt_cg_subr_rw_read_held,
> [DIF_SUBR_RW_WRITE_HELD] = &dt_cg_subr_rw_write_held,
> [DIF_SUBR_RW_ISWRITER] = &dt_cg_subr_rw_iswriter,
> - [DIF_SUBR_COPYIN] = NULL,
> + [DIF_SUBR_COPYIN] = &dt_cg_subr_copyin,
> [DIF_SUBR_COPYINSTR] = NULL,
> [DIF_SUBR_SPECULATION] = &dt_cg_subr_speculation,
> [DIF_SUBR_PROGENYOF] = &dt_cg_subr_progenyof,
> diff --git a/test/unittest/funcs/copyin/err.badaddr.d b/test/unittest/funcs/copyin/err.badaddr.d
> new file mode 100644
> index 00000000..a324d37d
> --- /dev/null
> +++ b/test/unittest/funcs/copyin/err.badaddr.d
No big deal, but I'm curious what's the difference between
test/unittest/funcs/copyin/err.badarg.d
test/unittest/funcs/copyin/err.null_arg1.d
Also out of curiosity (I guess I should know this), how do we know
0x1234 is a bad address?
> @@ -0,0 +1,24 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: Using copyin() with an invalid source address reports a fault.
> + *
> + * SECTION: Actions and Subroutines/copyin()
> + * User Process Tracing/copyin() and copyinstr()
> + */
> +
> +BEGIN
> +{
> + copyin(0x1234, 8);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/copyin/err.badaddr.r b/test/unittest/funcs/copyin/err.badaddr.r
> new file mode 100644
> index 00000000..ba4a4695
> --- /dev/null
> +++ b/test/unittest/funcs/copyin/err.badaddr.r
> @@ -0,0 +1,6 @@
> + FUNCTION:NAME
> + :ERROR
> +
> +-- @@stderr --
> +dtrace: script 'test/unittest/funcs/copyin/err.badaddr.d' matched 2 probes
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
> diff --git a/test/unittest/funcs/copyin/tst.copyin.d b/test/unittest/funcs/copyin/tst.copyin.d
> new file mode 100644
> index 00000000..52531cea
> --- /dev/null
> +++ b/test/unittest/funcs/copyin/tst.copyin.d
The test test/unittest/funcs/copyin/tst.copyin.d should ideally check
the output. I know it's hard, but the test is kind of toothless otherwise.
> @@ -0,0 +1,24 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2006, 2022, 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: It is possible to read data from the envp[] userspace area.
> + *
> + * SECTION: Actions and Subroutines/copyin()
> + * User Process Tracing/copyin() and copyinstr() Subroutines
> + */
> +
> +BEGIN
> +{
> + printf("envp[0] = \"%s\"", (string)copyin(curthread->mm->env_start, 256));
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
More information about the DTrace-devel
mailing list