[DTrace-devel] [PATCH 4/4] spec: correct semantics of specsize overflow
Eugene Loh
eugene.loh at oracle.com
Thu May 11 18:44:47 UTC 2023
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with usual comments/questions.
On 5/9/23 03:19, Kris Van Hees via DTrace-devel wrote:
> The speculations implementation (commit 7fe6ce9a "consume, cg: implement
> speculations") did not detect speculation data exceeding the specsize
> limit until the consumer added data fragments to a speculation buffer.
> As a result, all statements in clauses were always executed.
>
> The legacy behaviour is to abort clause execution once a data recording
> action causes the speculation buffer to overflow. That causes any
> further statementa in the clause to be skipped. The testsuite contains
statementa
->
statements
> an explicit test for this: tst.SpecSizeVariations4.d. Unfortunately,
> the test (and expected results) were amended to match the (incorrect)
> new semantics.
>
> This patch restores the legacy behaviour by tracking the size of the
> speculation across clauses that contribute to a given speculation, so
> that we can detect speculation buffer overflows as they occur due to
> a specific data recording action.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>
> diff --git a/bpf/speculation.c b/bpf/speculation.c
> index 9f3c0e6c..04b1d3f0 100644
> --- a/bpf/speculation.c
> +++ b/bpf/speculation.c
> @@ -105,29 +105,32 @@ noinline uint32_t dt_speculation(void)
> * Begin a speculation given an already-assigned ID.
> *
> * We consider a speculation ID usable only if it exists in the speculation map
> - * (indicating that speculation() has returned it) with a a zero read value
> + * (indicating that speculation() has returned it) and it is not being drained
> * (indicating that neither commit() nor discard() have been called for it).
> * We bump the written value by one to indicate that another speculative buffer
> * is (or will soon be, once this clause terminates and its epilogue runs)
> * available for draining.
> + *
> + * We return the speculation entry in the map or 0.
> */
How about NULL instead of 0? Also, since the function is so short and
well documented, the lead comment block might be unnecessary.
> -noinline int32_t
> +noinline dt_bpf_specs_t *
> dt_speculation_speculate(uint32_t id)
> {
> dt_bpf_specs_t *spec;
>
> if ((spec = bpf_map_lookup_elem(&specs, &id)) == NULL)
> - return -1;
> + return 0;
How about NULL instead of 0?
> /*
> * Spec already being drained: do not continue to emit new
> * data into it.
> */
> if (spec->draining)
> - return -1;
> + return 0;
How about NULL instead of 0?
> atomic_add(&spec->written, 1);
> - return 0;
> +
> + return spec;
> }
>
> /*
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -813,7 +815,8 @@ dt_cg_prologue(dt_pcb_t *pcb, dt_node_t *pred)
> TRACE_REGSET("Prologue: End ");
>
> /*
> - * Account for 32-bit epid (at offset 0) and 32-bit tag (at offset 4).
> + * Account for 32-BIt EPID (at offset 0) and 32-bit speculation ID (at
> + * offset 4).
> */
> pcb->pcb_bufoff += 2 * sizeof(uint32_t);
> }
Capitalized BIt?
> @@ -994,8 +1012,6 @@ dt_cg_check_fault(dt_pcb_t *pcb)
> emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_MST));
> emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DMST_FAULT));
> emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, reg, 0, lbl_ok));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_0, 0));
> - emit(dlp, BPF_RETURN());
> emitl(dlp, lbl_ok,
> BPF_NOP());
> dt_regset_free(drp, reg);
What? Is this change a mistake?
> @@ -1407,10 +1424,48 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> +ok:
> + /*
> + * If this clause contains a speculate() statement, we need to
> + * generate code to verify that the amount of data recorded for this
> + * speculation (incl. what was just added) does not exceed the
> + * specsize limit. If it does, further execution of this clause
> + * should be aborted.
> + */
> + if (cflags & DT_CLSFLAG_SPECULATE) {
> + uint_t lbl_ok = dt_irlist_label(dlp);
> + size_t specsize = dtp->dt_options[DTRACEOPT_SPECSIZE];
> +
> + /*
> + * if (*((uint32_t *)&buf[DBUF_SPECID]) != 0) {
> + * if (dctx->dmst->specsize + off + size >
dctx->dmst->specsize + off + size
->
dctx->dmst->specsize + (off + size)
> + * dtp->dt_options[DTRACEOPT_SPECSIZE])
> + * return;
> + * }
> + */
> + dt_regset_xalloc(drp, BPF_REG_0);
Why hardwire to %r0??? Why not just let dt_regset_alloc() find one for you?
> + emit(dlp, BPF_LOAD(BPF_W, BPF_REG_0, BPF_REG_9, DBUF_SPECID));
> + emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_ok));
> +
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DCTX_MST));
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_0, DMST_SPECSIZE));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, off + size));
> + emit(dlp, BPF_BRANCH_IMM(BPF_JLE, BPF_REG_0, specsize, lbl_ok));
How about changing
if (reg <= specsize) goto lbl_ok
to
if (reg > specsize) goto pcb_exitlbl
That would be simpler and map better to epilogue and your check_fault()
patch.
> + emit(dlp, BPF_MOV_IMM(BPF_REG_0, 0));
> + emit(dlp, BPF_RETURN());
> + dt_regset_free(drp, BPF_REG_0);
> +
> + emitl(dlp, lbl_ok,
> + BPF_NOP());
Are the regset_free() and lbl_ok out of order? E.g., let's say you jump
to lbl_ok. Then, if there were any reg fill code, you would miss it.
> + }
> +
> + return 0;
> }
>
> static void
> @@ -1973,14 +2029,16 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> /*
> - * rc = dt_speculation_speculate(spec);
> + * dt_bpf_specs_t *spec = dt_speculation_speculate(specid);
> * // lddw %r1, %dn_reg
> * // call dt_speculation_speculate
> * // (%r1 ... %r5 clobbered)
> * // (%r0 = error return)
%r0=error comment needs updating or removal (comment within a comment)
> - * if (rc != 0) // jne %r0, 0, lbl_exit
> + * if (spec == 0) // jne %r0, 0, lbl_exit
jne comment needs updating or removal (comment within a comment)
> * goto exit;
> - * *((uint32_t *)&buf[4]) = spec; // mov [%r9 + 4], %dn_reg
> + * *((uint32_t *)&buf[DBUF_SPECID]) = specid;
> + * // mov [%r9 + DBUF_SPECID], %dn_reg
> + * dctx->dmst->specsize = spec->size;
> * exit: // nop
> */
>
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> @@ -74,6 +76,21 @@ typedef struct dt_dctx {
>
> #define DCTX_SIZE ((int16_t)sizeof(dt_dctx_t))
>
> +/*
> + * The dctx->buf pointer references a block of memory that contains:
> + *
> + * +----------------+
> + * 0 -> | EPID |
> + * +----------------+
> + * 4 -> | Speculation ID |
Tab is inconsistent with the style of this comment block.
> + * +----------------+
> + * | Trace Data |
> + * | ... |
> + * +----------------+
> + */
> +#define DBUF_EPID 0
> +#define DBUF_SPECID 4
> +
> /*
> * The dctx->mem pointer references a block of memory that contains:
> *
> diff --git a/test/unittest/speculation/tst.SpecSizeVariations4.d b/test/unittest/speculation/tst.SpecSizeVariations4.d
Good, but:
1) The test (fwiw) was passing. Then in the last patch it failed. Now
it's passing again (this time properly). From a testing perspective,
there was a regression in this test in the previous patch. Ideally,
that would not be so. Maybe xfail this test in the previous patch, even
if that xfail is short-lived?
2) The long format strings are deceptive. They suggest that they are
the ones responsible for the overflow. How about getting rid of them. Or:
printf("%lld\n", x); /* Lots of data... */
printf("%lld\n", x); /* ... has to be crammed into this
buffer... */
printf("%lld\n", x); /* ... until it overflows... */
printf("%lld\n", x); /* ... and causes flops. */
3) The test is weak in that it checks only speculations in a single
clause. How about spreading the speculative data over multiple clauses?
More information about the DTrace-devel
mailing list