[DTrace-devel] [PATCH v3 3/3] String args to dt_strcmp() need to be copied if not DPTR
Eugene Loh
eugene.loh at oracle.com
Mon Oct 10 19:19:28 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
though:
*) I still think one can just use tmp2 directly without needing to
check flags and branch (unpleasant for the BPF verifier) simply to
access a single byte in t. As you point out, tmp2 gets overwritten, but
that overwriting is easily avoided by doing probe_read(tmp1, ..., tmp1).
*) For that matter, might also make sense to skip the flag check for s
as well. Just always use the helper instead in order to reduce
branching. That philosophy is already clearly present in strcmp() and
is what made strcmp() possible.
*) The test naming is odd in a couple of respects:
*) execname-cmp-bvar? But execname is a bvar. How about
execname-cmp-prbname?
*) execname-cmp-bvar_[lhs|rhs]? So lhs and rhs refer to what?
How about instead execname-cmp-bvar and bvar-cmp-execname and skip the
lhs/rhs labels.
*) How about more thorough comparisons and results testing? If the
tests are set up well, adding lots of checks (< <= dtrace dtracd dtracf
etc.) is just a matter of adding a few lines, and, in light of all the
mistakes that can come up, checking against a results file might be
really useful. If you like, I can post some tests.
On 10/8/22 13:19, Kris Van Hees via DTrace-devel wrote:
> The dt_strcmp() implementation makes use of indirect load instructions
> to retrieve characters from either string argument. That only works for
> DTrace-managed storage pointers. So, any argument that is not marked
> DT_NF_DPTR must be copied into a tstring before the indirect loads are
> done.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> bpf/strcmp.S | 52 ++++++++++++++-----
> libdtrace/dt_cg.c | 13 ++++-
> .../codegen/tst.execname-cmp-bvar_lhs.d | 19 +++++++
> .../codegen/tst.execname-cmp-bvar_rhs.d | 19 +++++++
> .../codegen/tst.execname-cmp-const_lhs.d | 19 +++++++
> .../codegen/tst.execname-cmp-const_rhs.d | 19 +++++++
> .../codegen/tst.kernel_str-cmp-bvar_lhs.d | 19 +++++++
> .../codegen/tst.kernel_str-cmp-bvar_rhs.d | 19 +++++++
> .../codegen/tst.kernel_str-cmp-const_lhs.d | 19 +++++++
> .../codegen/tst.kernel_str-cmp-const_rhs.d | 19 +++++++
> 10 files changed, 202 insertions(+), 15 deletions(-)
> create mode 100644 test/unittest/codegen/tst.execname-cmp-bvar_lhs.d
> create mode 100644 test/unittest/codegen/tst.execname-cmp-bvar_rhs.d
> create mode 100644 test/unittest/codegen/tst.execname-cmp-const_lhs.d
> create mode 100644 test/unittest/codegen/tst.execname-cmp-const_rhs.d
> create mode 100644 test/unittest/codegen/tst.kernel_str-cmp-bvar_lhs.d
> create mode 100644 test/unittest/codegen/tst.kernel_str-cmp-bvar_rhs.d
> create mode 100644 test/unittest/codegen/tst.kernel_str-cmp-const_lhs.d
> create mode 100644 test/unittest/codegen/tst.kernel_str-cmp-const_rhs.d
>
> diff --git a/bpf/strcmp.S b/bpf/strcmp.S
> index 75177469..49d1e64a 100644
> --- a/bpf/strcmp.S
> +++ b/bpf/strcmp.S
> @@ -58,12 +58,13 @@ dt_strcmp_not :
> .size dt_strcmp_not, .-dt_strcmp_not
>
> /*
> - * int dt_strcmp(char *s, char *t, char *tmp1, char *tmp2) {
> + * int dt_strcmp(char *s, char *t, char *tmp1, char *tmp2, uint64_t flags) {
> *
> * [%fp-8]=s
> * [%fp-16]=t
> * [%fp-24]=tmp1
> * [%fp-32]=tmp2
> + * r9=flags
> *
> * r8 = STRSZ
> *
> @@ -86,8 +87,14 @@ dt_strcmp_not :
> * // based on this location, judge if the strings are >, <, or ==
> * if (r0 > r6) goto Lsame;
> * if (r0 > r7) goto Lsame;
> - * if (s[r0] > t[r0]) return +1;
> - * if (s[r0] < t[r0]) return +1;
> + *
> + * s += r0;
> + * t += r0;
> + *
> + * if (flags & 1) { } else { bpf_probe_read(tmp1, 1, s); s = tmp1; }
> + * if (flags & 2) { } else { bpf_probe_read(tmp2, 1, t); t = tmp2; }
> + * if (s[0] > t[0]) return +1;
> + * return -1;
> *
> * // if all chars are the same, break tie on string length
> * Lsame:
> @@ -105,6 +112,7 @@ dt_strcmp :
> stxdw [%fp+-16], %r2 /* Spill t */
> stxdw [%fp+-24], %r3 /* Spill tmp1 */
> stxdw [%fp+-32], %r4 /* Spill tmp2 */
> + mov %r9, %r5 /* flags */
>
> lddw %r8, STRSZ /* r8 = STRSZ */
>
> @@ -156,22 +164,38 @@ dt_strcmp :
> sub %r0, 1 /* r0 -= 1 */
>
> jgt %r0, %r6, .Lsame /* if (r0 > r6) goto Lsame */
> - jgt %r0, %r8, .Lsame /* if (r0 > r8) goto Lsame */
> + jgt %r0, %r7, .Lsame /* if (r0 > r7) goto Lsame */
> +
> + ldxdw %r6, [%fp+-8]
> + add %r6, %r0 /* s += r0 */
> + ldxdw %r7, [%fp+-16]
> + add %r7, %r0 /* t += r0 */
> +
> + jset %r9, 1, .Ls_is_dptr
> + ldxdw %r1, [%fp+-24]
> + mov %r2, 1
> + mov %r3, %r6
> + mov %r6, %r1 /* s = tmp1 (after the helper call) */
> + call BPF_FUNC_probe_read /* bpf_probe_read(tmp1, 1, s) */
> +.Ls_is_dptr:
> +
> + jset %r9, 2, .Lt_is_dptr
> + ldxdw %r1, [%fp+-32]
> + mov %r2, 1
> + mov %r3, %r7
> + mov %r7, %r1 /* t = tmp2 (after the helper call) */
> + call BPF_FUNC_probe_read /* bpf_probe_read(tmp2, 1, t) */
> +.Lt_is_dptr:
>
> - ldxdw %r4, [%fp+-8]
> - add %r4, %r0
> - ldxb %r4, [%r4+0] /* s[r0] */
> - and %r4, 0xff
> + ldxb %r6, [%r6+0] /* s[0] */
> + and %r6, 0xff
>
> - ldxdw %r5, [%fp+-16]
> - add %r5, %r0
> - ldxb %r5, [%r5+0] /* t[r0] */
> - and %r5, 0xff
> + ldxb %r7, [%r7+0] /* t[0] */
> + and %r7, 0xff
>
> - jle %r4, %r5, 2 /* if (s[r0] > t[r0]) return +1 */
> + jle %r6, %r7, 2 /* if (s[0] > t[0]) return +1 */
> mov %r0, 1
> exit
> - jge %r4, %r5, 2 /* if (s[r0] < t[r0]) return +1 */
> mov %r0, -1
> exit
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index c4e49129..ef639161 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3294,6 +3294,7 @@ dt_cg_compare_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op)
> dt_node_is_string(dnp->dn_right)) {
> dt_ident_t *idp;
> uint_t Lcomp = dt_irlist_label(dlp);
> + uint32_t flags = 0;
> uint64_t off1, off2;
>
> /* if either pointer is NULL, just compare pointers */
> @@ -3301,7 +3302,16 @@ dt_cg_compare_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op)
> emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_right->dn_reg, 0, Lcomp));
>
> /*
> - * Otherwise, replace pointers with relative string values.
> + * Set flags to indicate which arguments (if any) are pointers
> + * to DTrace-managed storage.
> + */
> + if (dnp->dn_left->dn_flags & DT_NF_DPTR)
> + flags |= 1;
> + if (dnp->dn_right->dn_flags & DT_NF_DPTR)
> + flags |= 2;
> +
> + /*
> + * Replace pointers with relative string values.
> * Specifically, replace left with strcmp(left,right)+1 and
> * right with 1, so we can use unsigned comparisons.
> */
> @@ -3318,6 +3328,7 @@ dt_cg_compare_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op)
> emit(dlp, BPF_MOV_REG(BPF_REG_4, BPF_REG_3));
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, off1));
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, off2));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_5, flags));
> idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_strcmp");
> assert(idp != NULL);
> dt_regset_xalloc(drp, BPF_REG_0);
> diff --git a/test/unittest/codegen/tst.execname-cmp-bvar_lhs.d b/test/unittest/codegen/tst.execname-cmp-bvar_lhs.d
> new file mode 100644
> index 00000000..d36bbf07
> --- /dev/null
> +++ b/test/unittest/codegen/tst.execname-cmp-bvar_lhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(probename == execname);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.execname-cmp-bvar_rhs.d b/test/unittest/codegen/tst.execname-cmp-bvar_rhs.d
> new file mode 100644
> index 00000000..dcdf3507
> --- /dev/null
> +++ b/test/unittest/codegen/tst.execname-cmp-bvar_rhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(execname == probename);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.execname-cmp-const_lhs.d b/test/unittest/codegen/tst.execname-cmp-const_lhs.d
> new file mode 100644
> index 00000000..63e8d987
> --- /dev/null
> +++ b/test/unittest/codegen/tst.execname-cmp-const_lhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace("dtrace" == execname);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.execname-cmp-const_rhs.d b/test/unittest/codegen/tst.execname-cmp-const_rhs.d
> new file mode 100644
> index 00000000..e256f3a9
> --- /dev/null
> +++ b/test/unittest/codegen/tst.execname-cmp-const_rhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(execname == "dtrace");
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.kernel_str-cmp-bvar_lhs.d b/test/unittest/codegen/tst.kernel_str-cmp-bvar_lhs.d
> new file mode 100644
> index 00000000..ef95815a
> --- /dev/null
> +++ b/test/unittest/codegen/tst.kernel_str-cmp-bvar_lhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(probename == curthread->comm);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.kernel_str-cmp-bvar_rhs.d b/test/unittest/codegen/tst.kernel_str-cmp-bvar_rhs.d
> new file mode 100644
> index 00000000..1dd9a4ed
> --- /dev/null
> +++ b/test/unittest/codegen/tst.kernel_str-cmp-bvar_rhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(curthread->comm == probename);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.kernel_str-cmp-const_lhs.d b/test/unittest/codegen/tst.kernel_str-cmp-const_lhs.d
> new file mode 100644
> index 00000000..e877966e
> --- /dev/null
> +++ b/test/unittest/codegen/tst.kernel_str-cmp-const_lhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace("dtrace" == curthread->comm);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/codegen/tst.kernel_str-cmp-const_rhs.d b/test/unittest/codegen/tst.kernel_str-cmp-const_rhs.d
> new file mode 100644
> index 00000000..baebe17f
> --- /dev/null
> +++ b/test/unittest/codegen/tst.kernel_str-cmp-const_rhs.d
> @@ -0,0 +1,19 @@
> +/*
> + * 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.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(curthread->comm == "dtrace");
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
More information about the DTrace-devel
mailing list