[DTrace-devel] [PATCH 2/4] Fix syscall:::return args

[Kris Van Hees]

On Wed, May 04, 2022 at 01:01:42AM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_prov_syscall.c              | 30 +++++++----
>  test/unittest/syscall/tst.return_args.sh | 65 ++++++++++++++++++++++++
>  2 files changed, 86 insertions(+), 9 deletions(-)
>  create mode 100755 test/unittest/syscall/tst.return_args.sh
> 
> diff --git a/libdtrace/dt_prov_syscall.c b/libdtrace/dt_prov_syscall.c
> index 2c6644d4..a2e105eb 100644
> --- a/libdtrace/dt_prov_syscall.c
> +++ b/libdtrace/dt_prov_syscall.c
> @@ -159,19 +159,31 @@ static void trampoline(dt_pcb_t *pcb)
>  	dt_cg_tramp_clear_regs(pcb);
>  
>  	/*
> -	 *	for (i = 0; i < argc; i++)
> -	 *		dctx->mst->argv[i] =
> -	 *			((struct syscall_data *)dctx->ctx)->arg[i];
> -	 *				// lddw %r0, [%r8 + SCD_ARG(i)]
> -	 *				// stdw [%r7 + DMST_ARG(i)], %r0
> +	 * Copy in the probe args.
>  	 */
> -	for (i = 0; i < pcb->pcb_probe->argc; i++) {
> -		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, SCD_ARG(i)));
> -		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> +	if (strcmp(pcb->pcb_probe->desc->prb, "return") == 0) {
> +		/* the return value must be in both arg0 and arg1 */
> +		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, SCD_ARG(0)));
> +		for (i = 0; i < 2; i++)
> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> +	} else {
> +		/*
> +		 *	for (i = 0; i < argc; i++)
> +		 *		dctx->mst->argv[i] =
> +		 *			((struct syscall_data *)dctx->ctx)->arg[i];
> +		 *				// lddw %r0, [%r8 + SCD_ARG(i)]
> +		 *				// stdw [%r7 + DMST_ARG(i)], %r0
> +		 */
> +		for (i = 0; i < pcb->pcb_probe->argc; i++) {
> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, SCD_ARG(i)));
> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> +		}
>  	}
>  
>  	/*
> -	 *	for i = argc; i < ARRAY_SIZE(((dt_mstate_t *)0)->argv); i++)
> +	 * Zero the remaining probe args.
> +	 *
> +	 *	for ( ; i < ARRAY_SIZE(((dt_mstate_t *)0)->argv); i++)
>  	 *		dctx->mst->argv[i] = 0;
>  	 *				// stdw [%r7 + DMST_ARG(i)], 0
>  	 */

I think it is easier to simply make use of the fact that we already have
special case code in this function for return probes, and simply add one
instruction to that;

        if (strcmp(pcb->pcb_probe->desc->prb, "return") == 0) {
                uint_t lbl_errno_done = dt_irlist_label(dlp);

                emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ARG(0)));
+               emit(dlp,  BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
                emit(dlp,  BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_errno_done));

I think that better reflects that we take the arguments passed to us from the
underlying probe, and we then copy arg0 into arg1 to satisfy the documented
arguments for the return probe for syscalls.

> diff --git a/test/unittest/syscall/tst.return_args.sh b/test/unittest/syscall/tst.return_args.sh
> new file mode 100755
> index 00000000..14956598
> --- /dev/null
> +++ b/test/unittest/syscall/tst.return_args.sh
> @@ -0,0 +1,65 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2022, 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.
> +
> +dtrace=$1
> +
> +DIRNAME="$tmpdir/syscall.return_args.$$.$RANDOM"
> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +cat << EOF > main.c
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#define N 7
> +int main(int c, char **v) {
> +	int i, fds[N];
> +	for (i = 0; i < N; i++)
> +		printf("%d\n", fds[i] = open("/dev/zero", O_RDONLY));
> +	for (; i--; )
> +		close(fds[i]);
> +	return 0;
> +}
> +EOF
> +
> +/usr/bin/gcc main.c
> +if [ $? -ne 0 ]; then
> +	echo "compilation failure"
> +	exit 1
> +fi
> +
> +$dtrace $dt_flags -c ./a.out -o out.D -qZn '
> +syscall::open:return,
> +syscall::openat:return
> +/pid == $target/
> +{
> +	printf("%d\t%d\n", arg0, arg1);
> +}' > out.C 2>&1
> +
> +if [ $? -ne 0 ]; then
> +	echo "DTrace failure"
> +	cat out.C
> +	cat out.D
> +	exit 1
> +fi
> +
> +paste out.C out.C > out.cmp
> +echo >> out.cmp
> +diff -q out.D out.cmp
> +
> +if [ $? -ne 0 ]; then
> +	echo "diff failure"
> +	cat out.C
> +	cat out.cmp
> +	cat out.D
> +	diff out.D out.cmp
> +	exit 1
> +fi
> +
> +exit 0

I am curious...  Don't we already have a trigger that could be used for this,
e.g. open.c or bogus-ioctl.c?  Also, why are you explicitly adding output
verification when we have it built into the runtest.sh script by means of
$test.r files?  I'd rather see that used because it is the standard way we
compare with expected output.

> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel