* [PATCH 3/3] cg: fix masking of lower 32 bits @ 2025-09-23 21:05 Kris Van Hees 2025-09-23 22:30 ` [DTrace-devel] " Eugene Loh 0 siblings, 1 reply; 3+ messages in thread From: Kris Van Hees @ 2025-09-23 21:05 UTC (permalink / raw) To: dtrace, dtrace-devel The "and %r0, 0xffffffff' instruction does not actually mask off the lower 32 bits as one would expect because 0xffffffff is interpreted as -1 and then sign-extedned to a 64-bit value, i.e. keeping all bits. The "mov32 %r0, %r0" instruxtion does correctly mask off the lwoer 32 bits because it forced the value in %r0 to be a 32-bit value. Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com> --- include/bpf_asm.h | 11 +++++++++++ libdtrace/dt_cg.c | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/bpf_asm.h b/include/bpf_asm.h index 152d2312..63987e01 100644 --- a/include/bpf_asm.h +++ b/include/bpf_asm.h @@ -36,6 +36,15 @@ .imm = 0 \ }) +#define BPF_ALU32_REG(op, dst, src) \ + ((struct bpf_insn) { \ + .code = BPF_ALU | (op) | BPF_X, \ + .dst_reg = (dst), \ + .src_reg = (src), \ + .off = 0, \ + .imm = 0 \ + }) + #define BPF_END_REG(sz, dst, dir) \ ((struct bpf_insn) { \ .code = BPF_ALU | BPF_END | (dir), \ @@ -68,6 +77,8 @@ #define BPF_MOV_REG(dst, src) BPF_ALU64_REG(BPF_MOV, dst, src) #define BPF_MOV_IMM(dst, val) BPF_ALU64_IMM(BPF_MOV, dst, val) +#define BPF_MOV32_REG(dst, src) BPF_ALU32_REG(BPF_MOV, dst, src) + #define BPF_LOAD(sz, dst, src, ofs) \ ((struct bpf_insn) { \ .code = BPF_LDX | BPF_MEM | (sz), \ diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c index a8f2c9d2..28b7e7c4 100644 --- a/libdtrace/dt_cg.c +++ b/libdtrace/dt_cg.c @@ -2757,7 +2757,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk 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)); + /* mov32 %r0, %r0 effectively masks the lower 32 bits. */ + emit(dlp, BPF_MOV32_REG(BPF_REG_0, BPF_REG_0)); emit(dlp, BPF_STORE(BPF_DW, reg, off, BPF_REG_0)); dt_regset_free(drp, BPF_REG_0); } -- 2.43.5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH 3/3] cg: fix masking of lower 32 bits 2025-09-23 21:05 [PATCH 3/3] cg: fix masking of lower 32 bits Kris Van Hees @ 2025-09-23 22:30 ` Eugene Loh 2025-09-24 1:28 ` Kris Van Hees 0 siblings, 1 reply; 3+ messages in thread From: Eugene Loh @ 2025-09-23 22:30 UTC (permalink / raw) To: Kris Van Hees, dtrace, dtrace-devel On 9/23/25 17:05, Kris Van Hees via DTrace-devel wrote: > The "and %r0, 0xffffffff' instruction does not actually mask off the > lower 32 bits as one would expect because 0xffffffff is interpreted as > -1 and then sign-extedned to a 64-bit value, i.e. keeping all bits. s/extedned/extended/ > The "mov32 %r0, %r0" instruxtion does correctly mask off the lwoer 32 s/instruxtion/instruction/ s/lwoer/lower/ I started looking at this a while back and was not convinced that the lower 32 bits were what we want. Do we want them, and do we have a test that confirms? Or do we want the upper 32 bits? I got distracted and mothballed the whole thing, but the test involved spawning a child thread and then looking at its ustack()... maybe after the child had exited but before the parent did. Admittedly, I was confused at the time, and I'm even worse now. > bits because it forced the value in %r0 to be a 32-bit value. > > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com> > --- > include/bpf_asm.h | 11 +++++++++++ > libdtrace/dt_cg.c | 3 ++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/include/bpf_asm.h b/include/bpf_asm.h > index 152d2312..63987e01 100644 > --- a/include/bpf_asm.h > +++ b/include/bpf_asm.h > @@ -36,6 +36,15 @@ > .imm = 0 \ > }) > > +#define BPF_ALU32_REG(op, dst, src) \ > + ((struct bpf_insn) { \ > + .code = BPF_ALU | (op) | BPF_X, \ > + .dst_reg = (dst), \ > + .src_reg = (src), \ > + .off = 0, \ > + .imm = 0 \ > + }) > + > #define BPF_END_REG(sz, dst, dir) \ > ((struct bpf_insn) { \ > .code = BPF_ALU | BPF_END | (dir), \ > @@ -68,6 +77,8 @@ > #define BPF_MOV_REG(dst, src) BPF_ALU64_REG(BPF_MOV, dst, src) > #define BPF_MOV_IMM(dst, val) BPF_ALU64_IMM(BPF_MOV, dst, val) > > +#define BPF_MOV32_REG(dst, src) BPF_ALU32_REG(BPF_MOV, dst, src) > + > #define BPF_LOAD(sz, dst, src, ofs) \ > ((struct bpf_insn) { \ > .code = BPF_LDX | BPF_MEM | (sz), \ > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c > index a8f2c9d2..28b7e7c4 100644 > --- a/libdtrace/dt_cg.c > +++ b/libdtrace/dt_cg.c > @@ -2757,7 +2757,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk > 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)); > + /* mov32 %r0, %r0 effectively masks the lower 32 bits. */ > + emit(dlp, BPF_MOV32_REG(BPF_REG_0, BPF_REG_0)); > emit(dlp, BPF_STORE(BPF_DW, reg, off, BPF_REG_0)); > dt_regset_free(drp, BPF_REG_0); > } ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH 3/3] cg: fix masking of lower 32 bits 2025-09-23 22:30 ` [DTrace-devel] " Eugene Loh @ 2025-09-24 1:28 ` Kris Van Hees 0 siblings, 0 replies; 3+ messages in thread From: Kris Van Hees @ 2025-09-24 1:28 UTC (permalink / raw) To: Eugene Loh; +Cc: Kris Van Hees, dtrace, dtrace-devel On Tue, Sep 23, 2025 at 06:30:08PM -0400, Eugene Loh wrote: > On 9/23/25 17:05, Kris Van Hees via DTrace-devel wrote: > > > The "and %r0, 0xffffffff' instruction does not actually mask off the > > lower 32 bits as one would expect because 0xffffffff is interpreted as > > -1 and then sign-extedned to a 64-bit value, i.e. keeping all bits. > > s/extedned/extended/ Thanks. > > The "mov32 %r0, %r0" instruxtion does correctly mask off the lwoer 32 > > s/instruxtion/instruction/ > s/lwoer/lower/ Thanks. > I started looking at this a while back and was not convinced that the lower > 32 bits were what we want. Do we want them, and do we have a test that > confirms? Or do we want the upper 32 bits? I got distracted and mothballed > the whole thing, but the test involved spawning a child thread and then > looking at its ustack()... maybe after the child had exited but before the > parent did. Admittedly, I was confused at the time, and I'm even worse now. I leave that to you to decide :) All I do with this patch is accomplish what the code claimed to do (but failed to do). But since the consumer only looked at it as a 32-bit value, one wouldn't notice that the top 32 bits of the 64 bit word were not 0. In fact, with this patch, there is not a single change in test results because the code never would see the higher order bits. But since I plan on using those to store other info, I do need this masking to be done correctly. So this is a first step towards that. The code as written was a no-op. This patch fixes that. Without any regressions. And no, there is no test possible for this because the lack of masking would only be visible if something else was significantly broken in the consumer. > > bits because it forced the value in %r0 to be a 32-bit value. > > > > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com> > > --- > > include/bpf_asm.h | 11 +++++++++++ > > libdtrace/dt_cg.c | 3 ++- > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/include/bpf_asm.h b/include/bpf_asm.h > > index 152d2312..63987e01 100644 > > --- a/include/bpf_asm.h > > +++ b/include/bpf_asm.h > > @@ -36,6 +36,15 @@ > > .imm = 0 \ > > }) > > +#define BPF_ALU32_REG(op, dst, src) \ > > + ((struct bpf_insn) { \ > > + .code = BPF_ALU | (op) | BPF_X, \ > > + .dst_reg = (dst), \ > > + .src_reg = (src), \ > > + .off = 0, \ > > + .imm = 0 \ > > + }) > > + > > #define BPF_END_REG(sz, dst, dir) \ > > ((struct bpf_insn) { \ > > .code = BPF_ALU | BPF_END | (dir), \ > > @@ -68,6 +77,8 @@ > > #define BPF_MOV_REG(dst, src) BPF_ALU64_REG(BPF_MOV, dst, src) > > #define BPF_MOV_IMM(dst, val) BPF_ALU64_IMM(BPF_MOV, dst, val) > > +#define BPF_MOV32_REG(dst, src) BPF_ALU32_REG(BPF_MOV, dst, src) > > + > > #define BPF_LOAD(sz, dst, src, ofs) \ > > ((struct bpf_insn) { \ > > .code = BPF_LDX | BPF_MEM | (sz), \ > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c > > index a8f2c9d2..28b7e7c4 100644 > > --- a/libdtrace/dt_cg.c > > +++ b/libdtrace/dt_cg.c > > @@ -2757,7 +2757,8 @@ dt_cg_act_stack_sub(dt_pcb_t *pcb, dt_node_t *dnp, int reg, int off, dtrace_actk > > 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)); > > + /* mov32 %r0, %r0 effectively masks the lower 32 bits. */ > > + emit(dlp, BPF_MOV32_REG(BPF_REG_0, BPF_REG_0)); > > emit(dlp, BPF_STORE(BPF_DW, reg, off, BPF_REG_0)); > > dt_regset_free(drp, BPF_REG_0); > > } ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-09-24 1:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-23 21:05 [PATCH 3/3] cg: fix masking of lower 32 bits Kris Van Hees 2025-09-23 22:30 ` [DTrace-devel] " Eugene Loh 2025-09-24 1:28 ` 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