Re: [PATCH] trace: print alloca pointers as actual pointer values

On Fri, Aug 29, 2025 at 11:20:47PM -0400, Eugene Loh wrote:
> On 8/29/25 14:46, Kris Van Hees wrote:
> 
> > Because alloca pointers are stored internally as ofssets into the
> 
> s/ofssets/offsets/
> 
> For the test, how about converting the trace() to printf("%x")? Makes the
> script a little simpler and the hex output might feel more intuitive to a
> human reader if/when it ever comes to that.

The reason for using trace is that it is the most basic action that calls
dt_cg_store_val().  That is what we are exercising here.

> Most of all, the test fails wherever I tried it.  I think the problem is
> that the BPF verifier doesn't like adding a map_value pointer and an
> unbounded value... even if we do not dereference the result!  So, in this
> patch, drop the DT_NF_REF checks, instead calling
> dt_cg_alloca_access_check() everytime.

Ah, tehe joy of fixing one problem only to uncover another.  Dropping the
DT_NF_REF checks breaks other tests.  I'll have to see what needs to be done
to ensure that all tests pass correctly with a variant of this patch.

> I wrote a longer explanation of this.  Here it is, in case it's helpful.
> 
> The BPF verifier complains something like this:
> 
>     [...]
>     r8 = *(u64 *)(r10 -8) R8_w=fp-88          r8 = dctx
>     r8 = *(u64 *)(r8 +72) R8_w=map_value(...) r8 = dctx->gvar
>     r8 = *(u64 *)(r8 + 0) R8_w=inv(id=0)      r8 = arr
>     r7 = *(u64 *)(r10 -8) R7_w=fp-88          r7 = dctx
>     r7 = *(u64 *)(r7 +40) R7_w=map_value(...) r7 = dctx->scratchmem
>     r8 += r7              math between map_value pointer and register with
> unbounded min value is not allowed
> 
> So what has happened is that we assign r8=arr (first gvar), whose value is
> small since it's an alloca().  Then, we go to the new dt_cg.c code:
> 
>     if ((dnp->dn_flags & DT_NF_REF) || (arg & DT_NF_REF))
>         dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg, DT_ISIMM, size);
>     dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg, dnp->dn_reg);
> 
> We do NOT call the dt_cg_alloca_access_check(), but drop directly into
> dt_cg_alloca_ptr().  It allocates %r7 and does something like this:
> 
>     dt_cg_access_dctx(%r7, dlp, drp, DCTX_SCRATCHMEM);
>     emit(dlp,  BPF_ALU64_REG(BPF_ADD, %r8, %r7));
> 
> Anyhow, that last add is forbidden, because %r7 is dctx->scratchmem and %r8
> is unbounded.
> 
> I guess the problem is that the dt_cg_alloca_access_check() is not being
> called, and the BPF verifier doesn't like that, even though we are not
> dereferencing the pointer.
> 
> Maybe drop the &DT_NF_REF conditions in the new code introduced by the
> patch?

I think that is more likely that we need to scalarize the alloca pointer if
it not a REF, so that the arithmetic will be allowed (the verifier will no
longer know it is a map pointer).

> > scratchmem area, they were printed as small integers.  They are
> > now printed as actual pointer values into kernel space.
> > 
> > Signed-off-by: Kris Van Hees <[email protected]>
> > ---
> >   libdtrace/dt_cg.c                          | 19 +++++++++--------
> >   test/unittest/actions/trace/tst.alloca.d   | 24 ++++++++++++++++++++++
> >   test/unittest/actions/trace/tst.alloca.r   |  1 +
> >   test/unittest/actions/trace/tst.alloca.r.p | 11 ++++++++++
> >   4 files changed, 46 insertions(+), 9 deletions(-)
> >   create mode 100644 test/unittest/actions/trace/tst.alloca.d
> >   create mode 100644 test/unittest/actions/trace/tst.alloca.r
> >   create mode 100755 test/unittest/actions/trace/tst.alloca.r.p
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index cd9e7f4e9..7af3dd44b 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -1687,16 +1687,17 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   		align = vtype.dtdt_align;
> >   		/*
> > -		 * A DEREF of a REF node does not get resolved in dt_cg_node()
> > -		 * because the ref node already holds the pointer.  But for
> > -		 * alloca pointers, that will be the offset into scratchmem so
> > -		 * we still need to turn it into a real pointer here.
> > +		 * Alloca pointers are stored as an offset into scratchmem, so
> > +		 * they need to be converted into real pointers before we go on.
> > +		 * If the alloca pointer is a REF or ref-by-value is requested,
> > +		 * we need to do bounds checking before turning the alloca
> > +		 * pointer into a real pointer.
> >   		 */
> > -		if (dnp->dn_kind == DT_NODE_OP1 &&
> > -		    dnp->dn_op == DT_TOK_DEREF && (dnp->dn_flags & DT_NF_REF) &&
> > -		    (dnp->dn_child->dn_flags & DT_NF_ALLOCA)) {
> > -			dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg,
> > -						  DT_ISIMM, size);
> > +		if (dnp->dn_flags & DT_NF_ALLOCA) {
> > +			if ((dnp->dn_flags & DT_NF_REF) || (arg & DT_NF_REF))
> > +				dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg,
> > +							  DT_ISIMM, size);
> > +
> >   			dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg, dnp->dn_reg);
> >   			not_null = 1;
> >   		}
> > diff --git a/test/unittest/actions/trace/tst.alloca.d b/test/unittest/actions/trace/tst.alloca.d
> > new file mode 100644
> > index 000000000..d2ff5152d
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.alloca.d
> > @@ -0,0 +1,24 @@
> > +#pragma D option quiet
> > +
> > +BEGIN
> > +{
> > +	arr = (int *)alloca(5 * sizeof(int));
> > +	idx = 4;
> > +	arr[0] = 1;
> > +	arr[1] = 22;
> > +	arr[2] = 333;
> > +	arr[3] = 4444;
> > +	arr[4] = 55555;
> > +	trace(arr);
> > +	trace(" ");
> > +	trace(*arr);
> > +	trace(" ");
> > +	trace(arr + 2);
> > +	trace(" ");
> > +	trace(*(arr + 2));
> > +	trace(" ");
> > +	trace(arr + idx);
> > +	trace(" ");
> > +	trace(*(arr + idx));
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.alloca.r b/test/unittest/actions/trace/tst.alloca.r
> > new file mode 100644
> > index 000000000..e9bbf2f5d
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.alloca.r
> > @@ -0,0 +1 @@
> > +OK 1 OK 333 OK 55555
> > diff --git a/test/unittest/actions/trace/tst.alloca.r.p b/test/unittest/actions/trace/tst.alloca.r.p
> > new file mode 100755
> > index 000000000..8515861ad
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.alloca.r.p
> > @@ -0,0 +1,11 @@
> > +#!/usr/bin/gawk -f
> > +
> > +{
> > +	$1 = $1 > 0x7fffffff ? "OK" : "BAD";
> > +	$3 = $3 > 0x7fffffff ? "OK" : "BAD";
> > +	$5 = $5 > 0x7fffffff ? "OK" : "BAD";
> > +}
> > +
> > +{
> > +	print;
> > +}