* [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother
@ 2025-08-11 16:24 Kris Van Hees
2025-08-11 18:47 ` [DTrace-devel] " Eugene Loh
0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2025-08-11 16:24 UTC (permalink / raw)
To: dtrace, dtrace-devel
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@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
+}
+
+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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother
2025-08-11 16:24 [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother Kris Van Hees
@ 2025-08-11 18:47 ` Eugene Loh
2025-08-11 18:54 ` Kris Van Hees
0 siblings, 1 reply; 3+ messages in thread
From: Eugene Loh @ 2025-08-11 18:47 UTC (permalink / raw)
To: Kris Van Hees, dtrace, dtrace-devel
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother
2025-08-11 18:47 ` [DTrace-devel] " Eugene Loh
@ 2025-08-11 18:54 ` Kris Van Hees
0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2025-08-11 18:54 UTC (permalink / raw)
To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel
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 <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.)
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-11 18:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-11 16:24 [PATCH v2] dvar: ensure dynamic variables cannot overwrite eachother Kris Van Hees
2025-08-11 18:47 ` [DTrace-devel] " Eugene Loh
2025-08-11 18:54 ` Kris Van Hees
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).