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

[Eugene Loh <eugene.loh@oracle.com>]

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/

> 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/

> 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/

> and since that was the culprit, removing it resolves the issue.
>
> Test included.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> ---
>   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.)

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