[DTrace-devel] [PATCH v2 11/12] Add support for umod(), usym(), and uaddr()
Eugene Loh
eugene.loh at oracle.com
Fri Jun 11 18:28:24 PDT 2021
On 6/11/21 8:36 PM, Kris Van Hees wrote:
> Found something else that also applies to ustack... See below...
>
> On Fri, Jun 11, 2021 at 08:17:57PM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -739,13 +740,34 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>> + if (kind == DTRACEACT_USYM ||
>> + kind == DTRACEACT_UMOD ||
>> + kind == DTRACEACT_UADDR) {
>> + off = dt_rec_add(dtp, dt_cg_fill_gap, kind, 16, 8, NULL, arg);
>> +
>> + /* preface the value with the user process tgid */
>> + if (dt_regset_xalloc_args(drp) == -1)
>> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> + dt_regset_xalloc(drp, BPF_REG_0);
>> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
>> + dt_regset_free_args(drp);
>> + emit(dlp, BPF_STORE(BPF_W, BPF_REG_9, off, BPF_REG_0));
>> + dt_regset_free(drp, BPF_REG_0);
> The tgid is a 32-bit value that you are writing to a 64-bit wide space in the
> output buffer using a BPF_W store instruction, so it only touches 4 bytes.
> But that means that the other 8 bytes have undefined data in them. But then
> you read that in (dt_consume functions) as a 64-bit value. Most of the time
> that will work, but I can see situations where eventually you may run out of
> luck and there will be non-zero data in those other 4 bytes.
>
> The easiest solution would be to actually read the tgid as a 32-bit wide
> integer, ignoring the 4 padding bytes.
Thanks for noticing this. I'm fuzzy here: there are no endian issues
here, are there?
I suppose another approach might be to tgid&=0xffffffff?
> Alternatively (but slightly more complex in code) you could actually have two
> records for each U* action, one with a 4-byte data item, and one with a 8-byte
> data item. That is supported (since there are actually quite a few actions
> that have multiple data items associated with them), but again - slightly more
> complex even if it is 'cleaner'.
>
> Either way, I am pretty certain that you do need to do something about this
> 32-bit vs 64-bit data issue in view of the potential for non-zero data in the
> padding bytes.
That's interesting. I think I had (and tested) a BPF_DW version of one
of these patches, and it was fine. Presumably, it *definitely* had
nonzero bytes in that padding (because it called the BPF helper function
and then wrote all 8 bytes, which the consumer then read). The day might
have been saved by the consumer calling a function expecting a pid_t,
and so the garbage was coerced to 32 bits and one got shamefully lucky.
A dangerous way to live.
>
>> +
>> + /* then store the value */
>> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, off + 8, dnp->dn_reg));
>> + dt_regset_free(drp, dnp->dn_reg);
>> +
>> + return 0;
>> + }
More information about the DTrace-devel
mailing list