[DTrace-devel] [PATCH v3 06/19] alloca: add alloca() itself
Kris Van Hees
kris.van.hees at oracle.com
Thu Mar 31 19:14:33 UTC 2022
On Wed, Mar 30, 2022 at 01:05:17PM -0400, Kris Van Hees via DTrace-devel wrote:
> On Thu, Mar 24, 2022 at 06:24:32PM +0000, Nick Alcock via DTrace-devel wrote:
> > We add a new scratchsize option (default value 256, just like in days of
> > old), and a new scratchmem dctx pointer. This is allocated twice as
> > much space as we asked for, because the verifier can only use the values
> > of variables to adjust its bounds in very limited conditions: so we
> > leave enough space to write the full size of scratch space to the last
> > byte of scratch space without overflow. (Runtime checks will prohibit
> > such things, but the verifier doesn't know about those.)
>
> I would get rid of everything in the paragraph above except for the first
> sentence. The rest is not needed.
>
> > The top of currently-allocated scratch space is indicated with two
> > values in the machine state: DMST_SCRATCH_TOP, the current top of
> > scratch space, used for bounds-checking lookups, and DMST_SCRATCH_NEXT,
> > which is an 8-byte-aligned DMST_SCRATCH_TOP, and is where the next
> > allocation comes from. (Thus we ensure that all allocations are aligned
> > adequately for all D basic types, without losing bounds checking of the
> > most recently alloca()ed value.)
>
> Why not simply have DMST_SCRATCH_TOP be 8-byte-aligned and thereby avoid
> needing DMST_SCRATCH_NEXT? It is perfectly valid to over-allocate by a few
> bytes to ensure that the next allocation will always be properly aligned.
>
> Yes, it would mean that after you allocate 13 bytes, you are able to access
> 16 bytes, but that is fine. You just want to ensure that the scratchsize
> option value is also rounded up to the next 8-byte-boundary. When that is
> needed, you could emit a message saying that scratchsize got increased to the
> new value but perhaps that is not really necessary because it is just a small
> adjustment and I highly doubt anyone would count on it being exact (except when
> we write tests for exactly that and we do not really need to - just test based
> on the adjusted size).
>
> > Both values are reset to 0 before each clause is invoked.
> >
> > alloca itself is fairly pedestrian given all that. The size check is
> > fairly trivial: the only fiddly bit is the alignment (thanks to Kris Van
> > Hees for that).
>
> This becomes easier with the change mentioned above. Not that is was
> difficult before.
>
> > Overly large allocations are unconditionally refused via a compile-time
> > error, before even generating any code, preventing the verifier from
> > concluding that too-large allocations can't succeed and therefore
> > the success branch in the alloca size check is unreachable code.
>
> No need to mention the part about the verifier. That is subject to change
> anyway and might spark wondering minds (like mine) why that even matter since
> you would instead then do a size check in the generated code and avoid any
> verifier issues that way. So I would just simply avoid that thought process.
> It is valid in its own right to do this at compile time anyway.
>
> > alloca'ed pointers are not yet terribly usable at this stage: you can
> > use them directly but you can't assign them to variables, because
> > on retrieval the verifier will think they're just arbitrary scalars and
> > prohibit accesses through them. Thus there are no tests yet either.
> > (One new test fails temporarily:
> > test/unittest/codegen/tst.stack_layout.sh. It needs adjusting for the
> > new dmst variables, but we still have more to add in later commits.)
> >
> > Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> > ---
> > include/dtrace/options_defines.h | 3 +-
> > libdtrace/dt_bpf.c | 13 ++++
> > libdtrace/dt_cg.c | 104 ++++++++++++++++++++++++++++++-
> > libdtrace/dt_dctx.h | 24 ++++---
> > libdtrace/dt_dlibs.c | 1 +
> > libdtrace/dt_open.c | 6 ++
> > libdtrace/dt_options.c | 1 +
> > 7 files changed, 141 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/dtrace/options_defines.h b/include/dtrace/options_defines.h
> > index 5385f2f2dcde..337cd9d52e56 100644
> > --- a/include/dtrace/options_defines.h
> > +++ b/include/dtrace/options_defines.h
> > @@ -60,7 +60,8 @@
> > #define DTRACEOPT_BPFLOGSIZE 30 /* BPF verifier log, max # bytes */
> > #define DTRACEOPT_MAXFRAMES 31 /* maximum number of stack frames */
> > #define DTRACEOPT_BPFLOG 32 /* always output BPF verifier log */
> > -#define DTRACEOPT_MAX 33 /* number of options */
> > +#define DTRACEOPT_SCRATCHSIZE 33 /* max scratch size permitted */
> > +#define DTRACEOPT_MAX 34 /* number of options */
> >
> > #define DTRACEOPT_UNSET (dtrace_optval_t)-2 /* unset option */
> >
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index ade09b1528a1..cd217c76969e 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -204,6 +204,8 @@ populate_probes_map(dtrace_hdl_t *dtp, int fd)
> > * used to store the DTrace machine state, the temporary output
> > * buffer, and temporary storage for stack traces, string
> > * manipulation, etc.
> > + * - scratchmem: Storage for alloca() and other per-clause scratch space,
> > + * implemented just as for mem.
> > * - strtab: String table map. This is a global map with a singleton
> > * element (key 0) that contains the entire string table as a
> > * concatenation of all unique strings (each terminated with a
> > @@ -310,6 +312,17 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> > sizeof(uint32_t), memsz, 1) == -1)
> > return -1; /* dt_errno is set for us */
> >
> > + /*
> > + * The size for this is twice what it needs to be, to allow us to bcopy
>
> s/bcopy/copy/
>
> since this would also affect copy-by-value of large structs etc...
>
> > + * things the size of the scratch space to the start of the scratch
> > + * space without tripping verifier failures: see dt_cg_check_bounds.
> > + */
> > + if (create_gmap(dtp, "scratchmem", BPF_MAP_TYPE_PERCPU_ARRAY,
> > + sizeof(uint32_t),
> > + dtp->dt_options[DTRACEOPT_SCRATCHSIZE] *
> > + 2, 1) == -1)
> > + return -1; /* dt_errno is set for us */
> > +
> > /*
> > * We need to create the global (consolidated) string table. We store
> > * the actual length (for in-code BPF validation purposes) but augment
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 2c82ee9eb67b..32912880b97f 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -204,6 +204,7 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > } while(0)
> >
> > DT_CG_STORE_MAP_PTR("strtab", DCTX_STRTAB);
> > + DT_CG_STORE_MAP_PTR("scratchmem", DCTX_SCRATCHMEM);
> > if (dt_idhash_datasize(dtp->dt_aggs) > 0)
> > DT_CG_STORE_MAP_PTR("aggs", DCTX_AGG);
> > if (dt_idhash_datasize(dtp->dt_globals) > 0)
> > @@ -372,6 +373,15 @@ dt_cg_call_clause(dtrace_hdl_t *dtp, dt_ident_t *idp, dt_clause_arg_t *arg)
> > emit(dlp, BPF_LOAD(BPF_W, BPF_REG_0, BPF_REG_0, 0));
> > emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, arg->act, arg->lbl_exit));
> >
> > + /*
> > + * dctx.mst->scratch_top = 0;
> > + * // stw [%r7 + DMST_SCRATCH_TOP], 0
> > + * dctx.mst->scratch_next = 0;
> > + * // stw [%r7 + DMST_SCRATCH_NEXT], 0
> > + */
> > + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_SCRATCH_TOP, 0));
> > + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_7, DMST_SCRATCH_NEXT, 0));
>
> Per comments above, no need for *_NEXT and instead use *_TOP for that.
>
> > +
> > /*
> > * dt_clause(dctx); // mov %r1, %r9
> > * // call clause
> > @@ -3871,6 +3881,98 @@ dt_cg_subr_speculation(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > TRACE_REGSET(" subr-speculation: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;
> > + 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, scratchbot, top, next;
>
> Get rid of 'top' and just use 'next' per comments above. Seems contradictory
> but I think it makes more sense to work with a 'next' variable that will be
> used as ((*_TOP + size = 7) & -8) since that is really what you are needing.
> The old top value is in dnp->dn_reg.
>
> > +
> > + TRACE_REGSET(" subr-alloca:Begin");
> > +
> > + dt_cg_node(size, dlp, drp);
> > +
> > + /*
> > + * Compile-error out if the size is too large even absent other
> > + * allocations. (This prevents us generating code for which the
> > + * verifier will fail due to one branch of the conditional below being
> > + * determined to be unreachable.)
> > + */
> > + if (size->dn_kind == DT_NODE_INT && size->dn_value > opt_scratchsize)
> > + dnerror(dnp, D_ALLOCA_SIZE,
> > + "alloca(%lu) size larger than scratchsize %lu\n",
> > + (unsigned long) size->dn_value,
> > + (unsigned long) opt_scratchsize);
> > +
> > + if (((dnp->dn_reg = dt_regset_alloc(drp)) == -1) ||
> > + ((mst = dt_regset_alloc(drp)) == -1) ||
> > + ((scratchbot = dt_regset_alloc(drp)) == -1) ||
> > + ((top = dt_regset_alloc(drp)) == -1) ||
> > + ((next = dt_regset_alloc(drp)) == -1))
> > + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > +
> > + /*
> > + * %mst = DT_STK_DCTX->mst; // lddw %mst, [%fp + DCTX_FP(DCTX_MST)]
> > + * %scratchbot = DCTX_SCRATCHMEM; // lddw %scratchbot, [%fp + DT_STK_DCTX]
> > + * // add %scratchbot, DCTX_SCRATCHMEM
> > + * %dn_reg = DMST_SCRATCH_NEXT; // lddw %dn_reg, [%mst + DMST_SCRATCH_NEXT]
> > + * %top = %dn_reg + size; // mov %top, %dn_reg
> > + * // add %top, %size
> > + * %next = %top + 7 & -8; // mov %next, %top
> > + * // add %next, 7
> > + * // and %next, -8
> > + * if (%top > opt_scratchsize) || // jgt %top, opt_scratchsize, lbl_ok
> > + * (%scratchbot > opt_scratchsize) // jgt %scratchbot, opt_scratchsize, lbl_err
> > + * goto lbl_err;
> > + * goto lbl_ok;
> > + * lbl_err:
> > + * probe_error(DTRACEFLT_NOSCRATCH);
> > + *
> > + * lbl_ok:
> > + * DMST_SCRATCH_TOP = %top; // stdw [%mst + DMST_SCRATCH_TOP], %top
> > + * DMST_SCRATCH_NEXT = %next; // stdw [%mst + DMST_SCRATCH_NEXT], %next
> > + * %dn_reg += %scratchbot // add %dn_reg, %scratchbot
> > + * (result: %dn_reg)
> > + *
> > + */
>
> This comment block does not match the code.
>
> > +
> > + /* Loading. */
> > +
> > + emit(dlp, BPF_LOAD(BPF_DW, mst, BPF_REG_FP, DT_STK_DCTX));
> > + emit(dlp, BPF_LOAD(BPF_DW, mst, mst, DCTX_MST));
> > + emit(dlp, BPF_LOAD(BPF_DW, scratchbot, BPF_REG_FP, DT_STK_DCTX));
> > + emit(dlp, BPF_LOAD(BPF_DW, scratchbot, scratchbot, DCTX_SCRATCHMEM));
> > + emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, mst, DMST_SCRATCH_NEXT));
This is wrong. The scratch_next member in dt_mstate_s uint32_t so this
should be a BPF_W load. Even with the change to only have scratch_top this
needs to be a BPF_W load because that member is also uint32_t.
> > +
> > + /* Size testing and alignment. */
> > +
> > + emit(dlp, BPF_MOV_REG(top, dnp->dn_reg));
> > + emit(dlp, BPF_ALU64_REG(BPF_ADD, top, size->dn_reg));
> > + emit(dlp, BPF_MOV_REG(next, top));
> > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, next, 7));
> > + emit(dlp, BPF_ALU64_IMM(BPF_AND, next, (int) -8));
> > +
> > + emit(dlp, BPF_BRANCH_IMM(BPF_JGT, top, opt_scratchsize, lbl_err));
> > + emit(dlp, BPF_BRANCH_IMM(BPF_JGT, dnp->dn_reg, opt_scratchsize,
> > + lbl_err));
>
> You may want to add a comment here that this conditional will always be true,
> but that it is needed for the verifier to set proper bounds.
>
> > + emit(dlp, BPF_JUMP(lbl_ok));
> > + dt_cg_probe_error(yypcb, lbl_err, DTRACEFLT_NOSCRATCH, 0);
> > + emitl(dlp, lbl_ok,
> > + BPF_STORE(BPF_W, mst, DMST_SCRATCH_TOP, top));
> > + emit(dlp, BPF_STORE(BPF_W, mst, DMST_SCRATCH_NEXT, next));
> > + emit(dlp, BPF_ALU64_REG(BPF_ADD, dnp->dn_reg, scratchbot));
> > +
> > + dt_regset_free(drp, mst);
> > + dt_regset_free(drp, scratchbot);
> > + dt_regset_free(drp, top);
> > + dt_regset_free(drp, next);
> > + dt_regset_free(drp, size->dn_reg);
> > +
> > + TRACE_REGSET(" subr-alloca:End ");
> > +}
> > +
> > static void
> > dt_cg_subr_strchr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > {
> > @@ -4344,7 +4446,7 @@ static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
> > [DIF_SUBR_STRLEN] = &dt_cg_subr_strlen,
> > [DIF_SUBR_COPYOUT] = NULL,
> > [DIF_SUBR_COPYOUTSTR] = NULL,
> > - [DIF_SUBR_ALLOCA] = NULL,
> > + [DIF_SUBR_ALLOCA] = &dt_cg_subr_alloca,
> > [DIF_SUBR_BCOPY] = NULL,
> > [DIF_SUBR_COPYINTO] = NULL,
> > [DIF_SUBR_MSGDSIZE] = NULL,
> > diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> > index d08534fe1da2..df9cd4cde080 100644
> > --- a/libdtrace/dt_dctx.h
> > +++ b/libdtrace/dt_dctx.h
> > @@ -22,6 +22,8 @@ typedef struct dt_mstate {
> > uint32_t prid; /* Probe ID */
> > uint32_t clid; /* Clause ID (unique per probe) */
> > uint32_t tag; /* Tag (for future use) */
> > + uint32_t scratch_top; /* Current top of scratch space */
> > + uint32_t scratch_next; /* Next scratch space allocation */
> > int32_t syscall_errno; /* syscall errno */
> > uint64_t fault; /* DTrace fault flags */
> > uint64_t tstamp; /* cached timestamp value */
> > @@ -29,15 +31,17 @@ typedef struct dt_mstate {
> > uint64_t argv[10]; /* Probe arguments */
> > } dt_mstate_t;
> >
> > -#define DMST_EPID offsetof(dt_mstate_t, epid)
> > -#define DMST_PRID offsetof(dt_mstate_t, prid)
> > -#define DMST_CLID offsetof(dt_mstate_t, clid)
> > -#define DMST_TAG offsetof(dt_mstate_t, tag)
> > -#define DMST_ERRNO offsetof(dt_mstate_t, syscall_errno)
> > -#define DMST_FAULT offsetof(dt_mstate_t, fault)
> > -#define DMST_TSTAMP offsetof(dt_mstate_t, tstamp)
> > -#define DMST_REGS offsetof(dt_mstate_t, regs)
> > -#define DMST_ARG(n) offsetof(dt_mstate_t, argv[n])
> > +#define DMST_EPID offsetof(dt_mstate_t, epid)
> > +#define DMST_PRID offsetof(dt_mstate_t, prid)
> > +#define DMST_CLID offsetof(dt_mstate_t, clid)
> > +#define DMST_TAG offsetof(dt_mstate_t, tag)
> > +#define DMST_SCRATCH_TOP offsetof(dt_mstate_t, scratch_top)
> > +#define DMST_SCRATCH_NEXT offsetof(dt_mstate_t, scratch_next)
> > +#define DMST_ERRNO offsetof(dt_mstate_t, syscall_errno)
> > +#define DMST_FAULT offsetof(dt_mstate_t, fault)
> > +#define DMST_TSTAMP offsetof(dt_mstate_t, tstamp)
> > +#define DMST_REGS offsetof(dt_mstate_t, regs)
> > +#define DMST_ARG(n) offsetof(dt_mstate_t, argv[n])
> >
> > /*
> > * The DTrace context.
> > @@ -48,6 +52,7 @@ typedef struct dt_dctx {
> > dt_mstate_t *mst; /* DTrace machine state */
> > char *buf; /* Output buffer scratch memory */
> > char *mem; /* General scratch memory */
> > + char *scratchmem; /* Scratch space for alloca, etc */
> > char *strtab; /* String constant table */
> > char *agg; /* Aggregation data */
> > char *gvars; /* Global variables */
> > @@ -59,6 +64,7 @@ typedef struct dt_dctx {
> > #define DCTX_MST offsetof(dt_dctx_t, mst)
> > #define DCTX_BUF offsetof(dt_dctx_t, buf)
> > #define DCTX_MEM offsetof(dt_dctx_t, mem)
> > +#define DCTX_SCRATCHMEM offsetof(dt_dctx_t, scratchmem)
> > #define DCTX_STRTAB offsetof(dt_dctx_t, strtab)
> > #define DCTX_AGG offsetof(dt_dctx_t, agg)
> > #define DCTX_GVARS offsetof(dt_dctx_t, gvars)
> > diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> > index a6a5881ca509..e184babdac11 100644
> > --- a/libdtrace/dt_dlibs.c
> > +++ b/libdtrace/dt_dlibs.c
> > @@ -64,6 +64,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
> > DT_BPF_SYMBOL(lvars, DT_IDENT_PTR),
> > DT_BPF_SYMBOL(mem, DT_IDENT_PTR),
> > DT_BPF_SYMBOL(probes, DT_IDENT_PTR),
> > + DT_BPF_SYMBOL(scratchmem, DT_IDENT_PTR),
> > DT_BPF_SYMBOL(specs, DT_IDENT_PTR),
> > DT_BPF_SYMBOL(state, DT_IDENT_PTR),
> > DT_BPF_SYMBOL(strtab, DT_IDENT_PTR),
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index f11d54d8d55a..6442313483e4 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -608,6 +608,7 @@ static const char *_dtrace_libdir = DTRACE_LIBDIR; /* default library directory
> >
> > int _dtrace_strbuckets = 211; /* default number of hash buckets (prime) */
> > uint_t _dtrace_strsize = 256; /* default size of string intrinsic type */
> > +uint_t _dtrace_scratchsize = 256; /* default size of scratch space */
> > uint_t _dtrace_stkindent = 14; /* default whitespace indent for stack/ustack */
> > uint_t _dtrace_pidbuckets = 64; /* default number of pid hash buckets */
> > uint_t _dtrace_pidlrulim = 8; /* default number of pid handles to cache */
> > @@ -793,6 +794,11 @@ dt_vopen(int version, int flags, int *errp,
> > dtp->dt_options[DTRACEOPT_SPECSIZE] = 1024 * 1024 * 4;
> > dtp->dt_options[DTRACEOPT_NSPEC] = 1;
> >
> > + /*
> > + * Set the maximum scratch space permitted.
> > + */
> > + dtp->dt_options[DTRACEOPT_SCRATCHSIZE] = _dtrace_scratchsize;
>
> Round up to 8-byte boundary.
>
> > +
> > /*
> > * Set the default value of maxframes.
> > */
> > diff --git a/libdtrace/dt_options.c b/libdtrace/dt_options.c
> > index 62f356faeb17..82aac533aae3 100644
> > --- a/libdtrace/dt_options.c
> > +++ b/libdtrace/dt_options.c
> > @@ -1108,6 +1108,7 @@ static const dt_option_t _dtrace_rtoptions[] = {
> > { "maxframes", dt_opt_runtime, DTRACEOPT_MAXFRAMES },
> > { "nspec", dt_opt_runtime, DTRACEOPT_NSPEC },
> > { "pcapsize", dt_opt_pcapsize, DTRACEOPT_PCAPSIZE },
> > + { "scratchsize", dt_opt_size, DTRACEOPT_SCRATCHSIZE },
> > { "specsize", dt_opt_size, DTRACEOPT_SPECSIZE },
> > { "stackframes", dt_opt_runtime, DTRACEOPT_STACKFRAMES },
> > { "statusrate", dt_opt_rate, DTRACEOPT_STATUSRATE },
> > --
> > 2.35.1.261.g8402f930ba.dirty
> >
> >
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
> _______________________________________________
> 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