[DTrace-devel] [PATCH 10/12] Add support for copyin() subroutine
Kris Van Hees
kris.van.hees at oracle.com
Thu Jul 21 20:03:26 UTC 2022
On Thu, Jul 21, 2022 at 11:08:45AM -0700, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Thanks.
> 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?
Hm, good question. I usually indent based on how nested it is typically used.
I indent this one based on the fact that it is primarily used as the impl func
called from the alloca subroutine.
> How about allocating dnp->dn_reg inside of dt_cg_subr_alloca_impl() rather
> than have each caller do it?
I prefer to let the caller determine when and how the register is allocated
to allow more careful management of register allocation.
> > /*
> > * 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
One tests an address we cannot read from. The other tests using NULL (which is
usually handled separately in DTrace).
> Also out of curiosity (I guess I should know this), how do we know 0x1234 is
> a bad address?
As far as I recall, the first page is always reserved and not readable.
> > @@ -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.
Well, yes and no. The main point we're testing here is that we *can* read
the data. I would like to come up with a test where I can also verify the
correctness of the data but thus far I have not found a truly reproducible
way for doing that.
> > @@ -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);
> > +}
>
> _______________________________________________
> 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