Re: [PATCH v2] Fix trampoline use of BPF stack for scratch

[Kris Van Hees <kris.van.hees@oracle.com>]

On Thu, Jan 16, 2025 at 06:14:14PM -0500, eugene.loh@oracle.com wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> The trampoline starts by setting %r9 = %fp + -DCTX_SIZE -- that is,
> %r9 = %fp - 88.  Then, it fills the BPF stack from %fp-88 to %fp with
> the various dctx-> pointers.  Calls to clauses will reset %fp.
> 
> The proc provider's trampoline was using the BPF stack for scratch
> space for the exit and signal-handle probes.  Specifically, it used
> %fp + DT_STK_SPILL(0), which overwrites a dctx-> pointer.
> 
> Switch to DT_TRAMP_SP_SLOT(0), which is intended for this purpose.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
>  libdtrace/dt_prov_proc.c                         | 14 +++++++-------
>  test/unittest/lquantize/tst.32bit-bug26268136.sh |  1 -
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/libdtrace/dt_prov_proc.c b/libdtrace/dt_prov_proc.c
> index 0223c5d18..9fc499aeb 100644
> --- a/libdtrace/dt_prov_proc.c
> +++ b/libdtrace/dt_prov_proc.c
> @@ -62,7 +62,7 @@ static const char		modname[] = "vmlinux";
>   * The dependent probe support should include a priority specification to drive
>   * the order in which dependent probes are added to the underlying probe.  This
>   * is needed to enforce specific probe firing semantics (e.g. proc:::start must
> - * always precede [roc:::lwp-start).
> + * always precede proc:::lwp-start).
>   */
>  
>  typedef struct probe_arg {
> @@ -350,12 +350,12 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  		 *	else args[0] = 2;			// CLD_KILLED
>  		 */
>  		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> -		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_STK_SPILL(0)));
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_SLOT(0)));
>  		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, sizeof(int)));
>  		emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_3, BPF_REG_7, DMST_ARG(0)));
>  		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
>  		emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_probe_read));
> -		emit(dlp,  BPF_LOAD(BPF_W, BPF_REG_1, BPF_REG_FP, DT_STK_SPILL(0)));
> +		emit(dlp,  BPF_LOAD(BPF_W, BPF_REG_1, BPF_REG_FP, DT_TRAMP_SP_SLOT(0)));
>  		emit(dlp,  BPF_MOV_IMM(BPF_REG_0, 1));
>  		emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_1));
>  		emit(dlp,  BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 0x7f));
> @@ -406,17 +406,17 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>  		emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
>  		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, off));
>  		emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> -		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_STK_SPILL(0)));
> +		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_SLOT(0)));
>  		emit(dlp, BPF_MOV_IMM(BPF_REG_2, sz));
>  		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> -		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_3, BPF_REG_FP, DT_STK_SPILL(0)));
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_3, BPF_REG_FP, DT_TRAMP_SP_SLOT(0)));
>  		off = dt_cg_ctf_offsetof("struct signal_struct", "group_exit_code", &sz, 0);
>  		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, off));
>  		emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> -		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_STK_SPILL(0)));
> +		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_SLOT(0)));
>  		emit(dlp, BPF_MOV_IMM(BPF_REG_2, sz));
>  		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> -		emit(dlp, BPF_LOAD(BPF_W, BPF_REG_0, BPF_REG_FP, DT_STK_SPILL(0)));
> +		emit(dlp, BPF_LOAD(BPF_W, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_SLOT(0)));
>  		emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_keep));
>  		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
>  
> diff --git a/test/unittest/lquantize/tst.32bit-bug26268136.sh b/test/unittest/lquantize/tst.32bit-bug26268136.sh
> index d5f143f58..a360fd17e 100755
> --- a/test/unittest/lquantize/tst.32bit-bug26268136.sh
> +++ b/test/unittest/lquantize/tst.32bit-bug26268136.sh
> @@ -5,7 +5,6 @@
>  # Licensed under the Universal Permissive License v 1.0 as shown at
>  # http://oss.oracle.com/licenses/upl.
>  #
> -# @@xfail: dtv2
>  
>  if [ $# != 1 ]; then
>  	echo expected one argument: '<'dtrace-path'>'
> -- 
> 2.43.5
>