[DTrace-devel] [PATCH v3 3/3] String args to dt_strcmp() need to be copied if not DPTR
Kris Van Hees
kris.van.hees at oracle.com
Tue Oct 11 20:45:39 UTC 2022
On Mon, Oct 10, 2022 at 03:19:28PM -0400, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Thanks.
> 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).
Yes, but I wasn't touching the existing implementation since that was not
in the scope of this patch. Based on your feedback, I'll happily make that
change.
> *) 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.
I'd rather not because the reason you copy the string earlier in the function
is mostly because you need to have scratch copies to modify, which is valid,
but quite different from the need here in case of kernel strings. DTrace
managed storage should be accessible through direct and indirect loads and by
actually using those when possible we enable the BPF verifier to provide better
validation of the BPF code we generate. That is a benefit. Also, a helper
call *is* more expensive than a regular instruction.
> *) The test naming is odd in a couple of respects:
>
> *) execname-cmp-bvar? But execname is a bvar. How about
> execname-cmp-prbname?
The point is that execname is a special bvar. The bvar we compare with is not
relevant in terms of exercising the functionality.
> *) 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.
I think the test itself shows clearly which is which. I don't expect the name
to be perfectly self explanatory because for many tests that is simply not
possible. I do tend to name tests so that the main thing being tested
(execname, in this case) is listed first.
> *) 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.
I certainly encourage submitting additional tests for the strcmp() function.
But this particular patch does not have to do with the actual correctness of
the strcmp() implementation but rather with the fact that the code fails to
pass the BPF verifier when kernel strings are used. I.e. a code generation
issue, which is why the tests are in unittest/codegen.
>
> 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);
> > +}
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list