Re: [DTrace-devel] [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother

On Mon, Aug 11, 2025 at 02:47:36PM -0400, Eugene Loh wrote:
> Wow:  nasty bug, nice trouble-shooting, simple fix.
> 
> A few comments below on the test.
> 
> On 8/11/25 12:24, Kris Van Hees via DTrace-devel wrote:
> > As Eugene discovered, it was possible for dynamic variables (elements
> > of associative arrays and TLS variables) to overwrite eachother.  The
> > problem turns out to be due ot the implementation of the BPF helper
> 
> s/ot/to/

Thanks.  Fixed.

> > bpf_map_update_elem().  In order for the update to be seen as an atomic
> > operation, it does not update the balue of the map element in-place but
> 
> s/balue/value/

Thanks.  Fixed.

> > instead allocates a new element and places it in front of the old one
> > before it removes the old one.  The result is that the address of the
> > map element changes as a result of the bpf_map_update_elem() call.
> > 
> > Fortunately, we can just assign the address of the map element in the
> > value that we obtained using bpf_map_lookup_elem() because that gives
> > us a pointer to the map value, and we can assign directly into it.
> > 
> > In other words, we do not need the 2nd bpg_map_update_elem() anyway,
> 
> s/bpg/bpf/

Thanks.  Fixed.

> > and since that was the culprit, removing it resolves the issue.
> > 
> > Test included.
> > 
> > Signed-off-by: Kris Van Hees <[email protected]>
> > Signed-off-by: Eugene Loh <[email protected]>
> > ---
> >   bpf/get_dvar.c                       |  5 +----
> >   test/triggers/Build                  |  5 +++--
> >   test/triggers/pid-tst-timer.c        | 26 ++++++++++++++++++++++
> >   test/unittest/assocs/tst.collision.d | 33 ++++++++++++++++++++++++++++
> >   4 files changed, 63 insertions(+), 6 deletions(-)
> >   create mode 100644 test/triggers/pid-tst-timer.c
> >   create mode 100644 test/unittest/assocs/tst.collision.d
> > 
> > diff --git a/bpf/get_dvar.c b/bpf/get_dvar.c
> > index 1bb5eb002..073cca57c 100644
> > --- a/bpf/get_dvar.c
> > +++ b/bpf/get_dvar.c
> > @@ -1,6 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   /*
> > - * Copyright (c) 2019, 2024, Oracle and/or its affiliates.
> > + * Copyright (c) 2019, 2025, Oracle and/or its affiliates.
> >    */
> >   #include <linux/bpf.h>
> >   #include <stdint.h>
> > @@ -150,9 +150,6 @@ noinline void *dt_get_assoc(uint32_t id, const char *tuple, uint64_t store,
> >   		if (valp == 0)
> >   			return dt_no_dvar();
> >   		*valp = (uint64_t)valp;
> > -		if (bpf_map_update_elem(&tuples, tuple, valp, BPF_ANY) < 0)
> > -			return dt_no_dvar();
> > -
> >   		val = *valp;
> >   	} else {
> >   		/*
> > diff --git a/test/triggers/Build b/test/triggers/Build
> > index d49b996a6..30991db16 100644
> > --- a/test/triggers/Build
> > +++ b/test/triggers/Build
> > @@ -5,8 +5,8 @@
> >   EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie futex \
> >       periodic_output \
> > -    pid-tst-args1 pid-tst-float pid-tst-fork pid-tst-gcc \
> > -    pid-tst-ret1 pid-tst-ret2 pid-tst-vfork pid-tst-weak1 pid-tst-weak2 \
> > +    pid-tst-args1 pid-tst-float pid-tst-fork pid-tst-gcc pid-tst-ret1 \
> > +    pid-tst-ret2 pid-tst-timer pid-tst-vfork pid-tst-weak1 pid-tst-weak2 \
> >       proc-tst-sigwait proc-tst-omp proc-tst-pthread-exec profile-tst-ufuncsort \
> >       raise-tst-raise1 raise-tst-raise2 raise-tst-raise3 syscall-tst-args \
> >       ustack-tst-basic ustack-tst-bigstack ustack-tst-bigstack-spin \
> > @@ -191,6 +191,7 @@ pid-tst-args1_CFLAGS := -O0
> >   pid-tst-fork_CFLAGS := -O0
> >   pid-tst-ret1_CFLAGS := -O0
> >   pid-tst-ret2_CFLAGS := -O0
> > +pid-tst-timer_CFLAGS := -O0
> >   pid-tst-weak1_CFLAGS := -O0
> >   pid-tst-weak2_CFLAGS := -O0
> >   profile-tst-ufuncsort_CFLAGS := -O0
> > diff --git a/test/triggers/pid-tst-timer.c b/test/triggers/pid-tst-timer.c
> > new file mode 100644
> > index 000000000..ae98ffbfb
> > --- /dev/null
> > +++ b/test/triggers/pid-tst-timer.c
> > @@ -0,0 +1,26 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#include <unistd.h>
> > +
> > +void
> > +foo(void)
> > +{
> > +#if 0
> > +	usleep(1 * 1000 * 1000);
> > +#else
> > +	usleep(1 * 1000);
> > +#endif
> 
> Do not need the "#if 0" branch.  In fact, do not need the usleep() at all. 
> Actually, do not need the trigger at all.
> 
> (The test I used had that stuff because I wanted to watch the progress of
> the hash map contents using bpftool.  But for the test, none of that
> complexity is needed.)

Ah good point - I thought that perhaps there was a timing issue involved
also.

> > +}
> > +
> > +int
> > +main(void)
> > +{
> > +	while (1)
> > +		foo();
> > +	return 0;
> > +}
> > diff --git a/test/unittest/assocs/tst.collision.d b/test/unittest/assocs/tst.collision.d
> > new file mode 100644
> > index 000000000..1dc0ec7e3
> > --- /dev/null
> > +++ b/test/unittest/assocs/tst.collision.d
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +/* @@runtest-opts: $_pid */
> > +/* @@trigger: pid-tst-timer */
> > +/* @@trigger-timing: before */
> 
> Trigger is not needed.
> 
> > +
> > +
> > +/*
> > + * ASSERTION: Dynamic variables do not overwrite eachother.
> > + */
> > +
> > +pid$1:a.out:foo:entry { n++ }
> > +pid$1:a.out:foo:entry / n == 1 / { fdepth[0x1234] = 1; }
> > +pid$1:a.out:foo:entry / n == 2 / { self->start[1] = 0x1111; }
> > +pid$1:a.out:foo:entry / n == 3 / { self->start[1] = 0; }
> > +pid$1:a.out:foo:entry / n == 4 / { self->start[2] = 0x2222; }
> > +
> > +pid$1:a.out:foo:entry
> > +/ n == 5 /
> > +{ printf("%x / %x\n", fdepth[0x1234], self->start[2]); }
> > +
> > +pid$1:a.out:foo:entry
> > +/ n == 5 && fdepth[0x1234] == 1 &&  self->start[2] == 0x2222/
> > +{ exit(0); }
> > +
> > +pid$1:a.out:foo:entry
> > +/ n == 5 /
> > +{ exit(1); }
> 
> The fdepth and start names are historical and not particularly descriptive. 
> Over all, I would just go with:
> 
> #pragma D option quiet
> BEGIN {
>         a[0x1234] = 1;
>         self->b[1] = 0x1111;
>         self->b[1] = 0;
>         self->b[2] = 0x2222;
>         printf("%x / %x\n", a[0x1234], self->b[2]);
>         exit(0);
> }
> 
> with no trigger and do the results checking with a .r results file.

Taking this since no timing issue involved.  Though I think I will have the
test do the verification itself rather than using a .r file, since it is
perfectly possible to do the verification in the D script, and that is just
easier.  (Less files to modify if we ever change things.)

Will send v3.