* [PATCH] trace: print alloca pointers as actual pointer values @ 2025-08-29 18:46 Kris Van Hees 2025-08-30 3:20 ` Eugene Loh 0 siblings, 1 reply; 3+ messages in thread From: Kris Van Hees @ 2025-08-29 18:46 UTC (permalink / raw) To: dtrace, dtrace-devel Because alloca pointers are stored internally as ofssets into the 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 <kris.van.hees@oracle.com> --- 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; +} -- 2.45.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] trace: print alloca pointers as actual pointer values 2025-08-29 18:46 [PATCH] trace: print alloca pointers as actual pointer values Kris Van Hees @ 2025-08-30 3:20 ` Eugene Loh 2025-08-30 3:45 ` Kris Van Hees 0 siblings, 1 reply; 3+ messages in thread From: Eugene Loh @ 2025-08-30 3:20 UTC (permalink / raw) To: Kris Van Hees, dtrace, dtrace-devel 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 <kris.van.hees@oracle.com> > --- > 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; > +} ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] trace: print alloca pointers as actual pointer values 2025-08-30 3:20 ` Eugene Loh @ 2025-08-30 3:45 ` Kris Van Hees 0 siblings, 0 replies; 3+ messages in thread From: Kris Van Hees @ 2025-08-30 3:45 UTC (permalink / raw) To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel 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 <kris.van.hees@oracle.com> > > --- > > 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; > > +} ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-30 3:45 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-29 18:46 [PATCH] trace: print alloca pointers as actual pointer values Kris Van Hees 2025-08-30 3:20 ` Eugene Loh 2025-08-30 3:45 ` Kris Van Hees
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.