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

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.

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.

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?

> 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;
> +}