[DTrace-devel] [PATCH v2 11/12] Add support for umod(), usym(), and uaddr()

[Kris Van Hees]

On Fri, Jun 11, 2021 at 09:28:24PM -0400, Eugene Loh wrote:
> 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?

Good question... I think there are none right now, but that doesn't mean that
if we were to support other architectures there might not be any.  After all,
if we write a 4-byte entity and the consumer is reading it as a 8-byte entity
the 4 bytes may end up at the wrong end of the 8-byte entity.

I think that the cleanest way would be to have 2 data records for these
actions (a 4-byte one for the tgid and a 8-byte one for the value), and then
have the consumer retrieve them based on the record descriptions.  That
ensures that the (out-of-band) metadata has the correct information.

> I suppose another approach might be to tgid&=0xffffffff?

Yes, but then you are deliberately using a 64-bit value to store a 32-bit
value, just because the consumer is reading it as a 64-bit value even though
it *knows* it is supposed to be a 32-bit value.  Might as well just store it
as 32-bit since we support different size integers in the output stream.

> > 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;
> >> +	}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel