* [PATCH] Preface usym/umod/uaddr with pid
@ 2024-11-19 23:51 eugene.loh
2024-12-08 0:26 ` Kris Van Hees
0 siblings, 1 reply; 2+ messages in thread
From: eugene.loh @ 2024-11-19 23:51 UTC (permalink / raw)
To: dtrace, dtrace-devel
From: Eugene Loh <eugene.loh@oracle.com>
Translation of user space addresses requires that we know which process
to use for the translation. We have been prefacing the address to
translate like this:
/* Preface the value with the user process tgid. */
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
One problem is that the IMM "and" operation is a no-op. The 0xffffffff
is promoted to a 64-bit -1, leading to no %r0 bits changing. Another
problem is that we are pulling out the tgid. This means that the
translation will fail. Further, even if the consumer knew which part
of the pid_tgid combination to use for address translation, keys for
aggregations could fail spuriously due to different tgids for the same
process.
Convert to
/* Preface the value with the user process pid. */
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
Also, in the umod test (which uncovered this problem), add ld-*.so to
the list of shared objects we're going to skip.
Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
libdtrace/dt_cg.c | 8 ++++----
test/unittest/profile-n/tst.umod.sh | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 461a68270..b07aaea3d 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -1647,13 +1647,13 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
kind == DTRACEACT_UADDR) {
off = dt_rec_add(dtp, dt_cg_fill_gap, kind, 16, 8, NULL, arg);
- /* preface the value with the user process tgid */
+ /* preface the value with the user process pid */
if (dt_regset_xalloc_args(drp) == -1)
longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
dt_regset_xalloc(drp, BPF_REG_0);
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
dt_regset_free_args(drp);
- emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
+ emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, off, BPF_REG_0));
dt_regset_free(drp, BPF_REG_0);
@@ -3823,13 +3823,13 @@ empty_args:
if (tuplesize < nextoff)
emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, nextoff - tuplesize));
- /* Preface the value with the user process tgid. */
+ /* Preface the value with the user process pid. */
if (dt_regset_xalloc_args(drp) == -1)
longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
dt_regset_xalloc(drp, BPF_REG_0);
emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
dt_regset_free_args(drp);
- emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
+ emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
emit(dlp, BPF_STORE(BPF_DW, treg, 0, BPF_REG_0));
dt_regset_free(drp, BPF_REG_0);
diff --git a/test/unittest/profile-n/tst.umod.sh b/test/unittest/profile-n/tst.umod.sh
index a5b1127ff..45d2b1e9b 100755
--- a/test/unittest/profile-n/tst.umod.sh
+++ b/test/unittest/profile-n/tst.umod.sh
@@ -53,7 +53,7 @@ if ! grep -wq 'bash' $tmpfile; then
fi
# Check that modules are unique. (Exclude shared libraries and unresolved addresses.)
-if gawk '!/^ *lib/ && !/^ *0x/ {print $1}' $tmpfile | sort | uniq -c | grep -qv " 1 "; then
+if gawk '!/^ *lib/ && !/^ *ld-.*\.so / && !/^ *0x/ {print $1}' $tmpfile | sort | uniq -c | grep -qv " 1 "; then
echo ERROR: duplicate umod
status=1
fi
--
2.43.5
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Preface usym/umod/uaddr with pid
2024-11-19 23:51 [PATCH] Preface usym/umod/uaddr with pid eugene.loh
@ 2024-12-08 0:26 ` Kris Van Hees
0 siblings, 0 replies; 2+ messages in thread
From: Kris Van Hees @ 2024-12-08 0:26 UTC (permalink / raw)
To: eugene.loh; +Cc: dtrace, dtrace-devel
On Tue, Nov 19, 2024 at 06:51:25PM -0500, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
>
> Translation of user space addresses requires that we know which process
> to use for the translation. We have been prefacing the address to
> translate like this:
> /* Preface the value with the user process tgid. */
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
>
> One problem is that the IMM "and" operation is a no-op. The 0xffffffff
> is promoted to a 64-bit -1, leading to no %r0 bits changing. Another
> problem is that we are pulling out the tgid. This means that the
> translation will fail. Further, even if the consumer knew which part
> of the pid_tgid combination to use for address translation, keys for
> aggregations could fail spuriously due to different tgids for the same
> process.
>
> Convert to
> /* Preface the value with the user process pid. */
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
>
> Also, in the umod test (which uncovered this problem), add ld-*.so to
> the list of shared objects we're going to skip.
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
> libdtrace/dt_cg.c | 8 ++++----
> test/unittest/profile-n/tst.umod.sh | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 461a68270..b07aaea3d 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1647,13 +1647,13 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> kind == DTRACEACT_UADDR) {
> off = dt_rec_add(dtp, dt_cg_fill_gap, kind, 16, 8, NULL, arg);
>
> - /* preface the value with the user process tgid */
> + /* preface the value with the user process pid */
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> dt_regset_free_args(drp);
> - emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
> + emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, off, BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
>
> @@ -3823,13 +3823,13 @@ empty_args:
> if (tuplesize < nextoff)
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, nextoff - tuplesize));
>
> - /* Preface the value with the user process tgid. */
> + /* Preface the value with the user process pid. */
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> dt_regset_free_args(drp);
> - emit(dlp, BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff));
> + emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> emit(dlp, BPF_STORE(BPF_DW, treg, 0, BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
>
> diff --git a/test/unittest/profile-n/tst.umod.sh b/test/unittest/profile-n/tst.umod.sh
> index a5b1127ff..45d2b1e9b 100755
> --- a/test/unittest/profile-n/tst.umod.sh
> +++ b/test/unittest/profile-n/tst.umod.sh
> @@ -53,7 +53,7 @@ if ! grep -wq 'bash' $tmpfile; then
> fi
>
> # Check that modules are unique. (Exclude shared libraries and unresolved addresses.)
> -if gawk '!/^ *lib/ && !/^ *0x/ {print $1}' $tmpfile | sort | uniq -c | grep -qv " 1 "; then
> +if gawk '!/^ *lib/ && !/^ *ld-.*\.so / && !/^ *0x/ {print $1}' $tmpfile | sort | uniq -c | grep -qv " 1 "; then
> echo ERROR: duplicate umod
> status=1
> fi
> --
> 2.43.5
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-12-08 0:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-19 23:51 [PATCH] Preface usym/umod/uaddr with pid eugene.loh
2024-12-08 0:26 ` 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