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

[Kris Van Hees]

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
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
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,
and since that was the culprit, removing it resolves the issue.

Test included.

Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
Signed-off-by: Eugene Loh <eugene.loh at 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
+}
+
+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 */
+
+
+/*
+ * 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); }
-- 
2.45.2