[DTrace-devel] [PATCH v3 06/19] alloca: add alloca() itself
Kris Van Hees
kris.van.hees at oracle.com
Wed Mar 30 17:05:17 UTC 2022
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));
> +
> + /* 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
More information about the DTrace-devel
mailing list