* [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 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.