[DTrace-devel] [PATCH 1/4] Implement strlen() based on bpf_probe_read_str()
Eugene Loh
eugene.loh at oracle.com
Fri Jan 28 06:39:43 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
On 1/26/22 11:28 AM, Kris Van Hees via DTrace-devel wrote:
> The bpf_probe_read_atr() BPF helper returns the number of characters
s/atr/str/
> it copies (incl. the terminating NUL byte), up to the maximum number
> passed as one of the arguments. This can be used to determine the
> length of a string.
First paragraph is okay. Or, if you want, just say:
"The bpf_probe_read_str() BPF helper return value
can be used to determine the length of a string."
> This implementation makes use of the unused space at the end of the
> string constant table, which is sized so that it can contain the
> largest string possible.
To me, "implementation" is unclear, since you were just talking about
bpf_probe_read_str(). To me, s/implementation/patch/ would be clearer.
This "end of strtab" strategy is a little funny. It is not wrong, but
it is slightly unsettling. Like, what if two CPUs are writing at once?
Granted, we're not reading the data, but this still feels odd. We
usually use tstrings for "this sort of thing."
> The implementation of substr) is adjusted to reflect this change as
> well. Rather than calling the strlen() function, it uses the BPF
> helper directly.
substr) should perhaps be substr()?
> This patch also introduces tests for the strlen() function.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> bpf/strlen.c | 19 ++++--
> bpf/substr.S | 17 +++--
> include/bpf-lib.h | 22 ++++++-
> libdtrace/dt_bpf.c | 8 +--
> libdtrace/dt_cg.c | 63 ++++++++-----------
> .../funcs/strlen/err.D_PROTO_ARG.non_string.d | 18 ++++++
> .../strlen/err.D_PROTO_LEN.missing_arg.d | 18 ++++++
> .../strlen/err.D_PROTO_LEN.too_many_args.d | 18 ++++++
> test/unittest/funcs/strlen/tst.basic.d | 37 +++++++++++
> test/unittest/funcs/strlen/tst.basic.r | 15 +++++
> test/unittest/funcs/strlen/tst.capped-sizw.d | 25 ++++++++
> .../strlen/tst.empty.d} | 15 +++--
> test/unittest/funcs/strlen/tst.null.d | 25 ++++++++
> test/unittest/funcs/strlen/tst.null.r | 3 +
> 14 files changed, 243 insertions(+), 60 deletions(-)
> create mode 100644 test/unittest/funcs/strlen/err.D_PROTO_ARG.non_string.d
> create mode 100644 test/unittest/funcs/strlen/err.D_PROTO_LEN.missing_arg.d
> create mode 100644 test/unittest/funcs/strlen/err.D_PROTO_LEN.too_many_args.d
> create mode 100644 test/unittest/funcs/strlen/tst.basic.d
> create mode 100644 test/unittest/funcs/strlen/tst.basic.r
> create mode 100644 test/unittest/funcs/strlen/tst.capped-sizw.d
> rename test/unittest/{strlen/tst.strlen1.d => funcs/strlen/tst.empty.d} (50%)
> create mode 100644 test/unittest/funcs/strlen/tst.null.d
> create mode 100644 test/unittest/funcs/strlen/tst.null.r
>
> diff --git a/bpf/strlen.c b/bpf/strlen.c
> index b2a8c740..889b2949 100644
> --- a/bpf/strlen.c
> +++ b/bpf/strlen.c
> @@ -1,8 +1,12 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
> */
> +#include <linux/bpf.h>
> +#include <bpf-helpers.h>
> #include <stdint.h>
> +#include <dt_dctx.h>
> +#include <bpf-lib.h>
>
> #define DT_STRLEN_BYTES 2
>
> @@ -10,6 +14,9 @@
> # define noinline __attribute__((noinline))
> #endif
>
> +extern uint64_t STBSZ;
> +extern uint64_t STRSZ;
> +
> noinline void dt_strlen_store(uint64_t val, char *str)
> {
> uint8_t *buf = (uint8_t *)str;
> @@ -18,9 +25,13 @@ noinline void dt_strlen_store(uint64_t val, char *str)
> buf[1] = (uint8_t)(val & 0xff);
> }
>
> -noinline uint64_t dt_strlen(const char *str)
> +noinline uint64_t dt_strlen(const dt_dctx_t *dctx, const char *str)
> {
> - const uint8_t *buf = (const uint8_t *)str;
> + char *tmp = dctx->strtab + (uint64_t)&STBSZ;
> + int64_t len;
> +
> + len = bpf_probe_read_str(tmp, (uint64_t)&STRSZ + 1, str + DT_STRLEN_BYTES);
> + set_not_neg_bound(len);
>
> - return ((uint64_t)buf[0] << 8) + (uint64_t)buf[1];
> + return len - 1; /* bpf_probe_read_str() never returns 0 */
> }
> diff --git a/bpf/substr.S b/bpf/substr.S
> index bc6133c9..a6cf5a57 100644
> --- a/bpf/substr.S
> +++ b/bpf/substr.S
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
> */
>
> #define DT_STRLEN_BYTES 2
> @@ -48,10 +48,17 @@ dt_substr :
> * the length we got. (The length is initialized in %r8 as the default
> * string length.)
> */
> - ldxdw %r1, [%fp+-8]
> - call dt_strlen /* len = dt_strlen(src) */
> - jeq %r0, 0, .Lempty /* if (len == 0) goto Lempty; */
> - jge %r0, %r8, .Llen_set /* if (len >= STRSZ) goto Llen_set; */
> + mov %r3, %r2
> + add %r3, DT_STRLEN_BYTES
> + mov %r2, %r8
> + add %r8, 1
Why %r8++? The updated value is never used. (Or maybe I'm missing
something.)
> + call BPF_FUNC_probe_read_str /*
> + * len = probe_read_str(
> + * dst, STRSZ + 1,
> + * &src[DT_STRLEN_BYTES]);
> + */
> + jslt %r0, 0, .Lempty /* if (len < 0) goto Lempty; */
We'll never get %r0==0, but still might as well s/jslt/jsle/.
Come to think of it, might as well make it "jsle 1" since, if there was
only a NUL char, we can jump to Lempty.
> + sub %r0, 1 /* len--; */
> mov %r8, %r0 /* %r8 = len */
> .Llen_set:
I think the .Llen_set label is now orphaned and can be removed.
> diff --git a/include/bpf-lib.h b/include/bpf-lib.h
> index f7885d2a..d7078da5 100644
> --- a/include/bpf-lib.h
> +++ b/include/bpf-lib.h
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2021, 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.
> */
> @@ -9,7 +9,7 @@
> #define BPF_LIB_H
>
> /*
> - * Explicit inline assembler to implement an upper bound check:
> + * Explicit inline assembler to implement a dynamic upper bound check:
> *
> * if (var > bnd)
> * var = bnd;
> @@ -28,7 +28,7 @@
> );
>
> /*
> - * Explicit inline assembler to implement a lower bound check:
> + * Explicit inline assembler to implement a dynamic lower bound check:
> *
> * if (var < bnd)
> * var = bnd;
> @@ -46,4 +46,20 @@
> : /* no clobbers */ \
> );
>
> +/*
> + * Explicit inline assembler to implement a non-negative bound check:
> + *
> + * if (var < 0)
> + * var = 0;
> + */
> +#define set_not_neg_bound(var) \
> + asm ("jsge %0, 0, 1f\n\t" \
> + "mov %0, 0\n\t" \
> + "1:" \
> + : "+r" (var) \
> + : /* no inputs */ \
> + : /* no clobbers */ \
> + );
> +
> +
> #endif /* BPF_LIB_H */
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 50343057..cfd7c6d7 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2019, 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2019, 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.
> */
> @@ -313,11 +313,11 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> /*
> * We need to create the global (consolidated) string table. We store
> * the actual length (for in-code BPF validation purposes) but augment
> - * it by the maximum string size to determine the size of the BPF map
> - * value that is used to store the strtab.
> + * it by the maximum string storage size to determine the size of the
> + * BPF map value that is used to store the strtab.
> */
> dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
> - stabsz = dtp->dt_strlen + strsize;
> + stabsz = dtp->dt_strlen + strsize + 1;
> strtab = dt_zalloc(dtp, stabsz);
> if (strtab == NULL)
> return dt_set_errno(dtp, EDT_NOMEM);
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index e233e757..0225832d 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -755,39 +755,6 @@ dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
> dt_regset_free(drp, BPF_REG_0);
> }
>
> -static void
> -dt_cg_strlen(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src)
> -{
> - dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_strlen");
> - size_t size = yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE];
> - uint_t lbl_ok = dt_irlist_label(dlp);
> -
> - TRACE_REGSET(" strlen:Begin");
> -
> - assert(idp != NULL);
> - if (dt_regset_xalloc_args(drp) == -1)
> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> -
> - if (src != BPF_REG_1)
> - emit(dlp, BPF_MOV_REG(BPF_REG_1, src));
> - if (dst != BPF_REG_0)
> - dt_regset_xalloc(drp, BPF_REG_0);
> -
> - emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> - dt_regset_free_args(drp);
> - emit(dlp, BPF_BRANCH_IMM(BPF_JLE, BPF_REG_0, size, lbl_ok));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_0, size));
> - emitl(dlp, lbl_ok,
> - BPF_NOP());
> -
> - if (dst != BPF_REG_0) {
> - emit(dlp, BPF_MOV_REG(dst, BPF_REG_0));
> - dt_regset_free(drp, BPF_REG_0);
> - }
> -
> - TRACE_REGSET(" strlen:End ");
> -}
> -
> static void
> dt_cg_spill_store(int reg)
> {
> @@ -3763,10 +3730,34 @@ dt_cg_subr_strrchr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> static void
> dt_cg_subr_strlen(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
> + dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_strlen");
> + dt_node_t *str = dnp->dn_args;
> +
> + assert(idp != NULL);
> +
> TRACE_REGSET(" subr-strlen:Begin");
> - dt_cg_node(dnp->dn_args, dlp, drp);
> - dnp->dn_reg = dnp->dn_args->dn_reg;
> - dt_cg_strlen(dlp, drp, dnp->dn_reg, dnp->dn_args->dn_reg);
> +
> + dt_cg_node(str, dlp, drp);
> + dt_cg_check_notnull(dlp, drp, str->dn_reg);
> + dnp->dn_reg = str->dn_reg; /* re-use register */
Why assign dnp->dn_reg? You allocate another register for it below and
never use the value assigned here.
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, str->dn_reg));
> + dt_regset_free(drp, str->dn_reg);
> + dt_cg_tstring_free(yypcb, str);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp,BPF_CALL_FUNC(idp->di_id), idp);
> +
> + dt_regset_free_args(drp);
> + dnp->dn_reg = dt_regset_alloc(drp);
> + if (dnp->dn_reg == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> + emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> +
> TRACE_REGSET(" subr-strlen:End ");
> }
>
> diff --git a/test/unittest/funcs/strlen/err.D_PROTO_ARG.non_string.d b/test/unittest/funcs/strlen/err.D_PROTO_ARG.non_string.d
> new file mode 100644
> index 00000000..a9f2a38c
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/err.D_PROTO_ARG.non_string.d
> @@ -0,0 +1,18 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: The argument to strlen() should be a string.
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +BEGIN
> +{
> + strlen(12);
> + exit(0);
> +}
> diff --git a/test/unittest/funcs/strlen/err.D_PROTO_LEN.missing_arg.d b/test/unittest/funcs/strlen/err.D_PROTO_LEN.missing_arg.d
> new file mode 100644
> index 00000000..9467240d
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/err.D_PROTO_LEN.missing_arg.d
> @@ -0,0 +1,18 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: strlen() requires an argument
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +BEGIN
> +{
> + strlen();
> + exit(0);
> +}
> diff --git a/test/unittest/funcs/strlen/err.D_PROTO_LEN.too_many_args.d b/test/unittest/funcs/strlen/err.D_PROTO_LEN.too_many_args.d
> new file mode 100644
> index 00000000..c0e8fd13
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/err.D_PROTO_LEN.too_many_args.d
> @@ -0,0 +1,18 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: strlen() accepts exactly one argument
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +BEGIN
> +{
> + strlen("", 1);
> + exit(0);
> +}
> diff --git a/test/unittest/funcs/strlen/tst.basic.d b/test/unittest/funcs/strlen/tst.basic.d
> new file mode 100644
> index 00000000..e2ae275b
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/tst.basic.d
> @@ -0,0 +1,37 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: The strlen() subroutine returns the correct result.
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +/* @@runtest-opts: -C */
> +
> +#pragma D option quiet
> +
> +#define TEST(s) printf("%60s %d\n", (s), strlen(s))
> +
> +BEGIN
> +{
> + TEST("1");
> + TEST("12");
> + TEST("123");
> + TEST("12345");
> + TEST("123456");
> + TEST("1234567");
> + TEST("12345678");
> + TEST("123456789");
> + TEST("1234567890");
> + TEST("12345678901234567890");
> + TEST("123456789012345678901234567890");
> + TEST("1234567890123456789012345678901234567890");
> + TEST("12345678901234567890123456789012345678901234567890");
> + TEST("123456789012345678901234567890123456789012345678901234567890");
> + exit(0);
> +}
> diff --git a/test/unittest/funcs/strlen/tst.basic.r b/test/unittest/funcs/strlen/tst.basic.r
> new file mode 100644
> index 00000000..822e67a6
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/tst.basic.r
> @@ -0,0 +1,15 @@
> + 1 1
> + 12 2
> + 123 3
> + 12345 5
> + 123456 6
> + 1234567 7
> + 12345678 8
> + 123456789 9
> + 1234567890 10
> + 12345678901234567890 20
> + 123456789012345678901234567890 30
> + 1234567890123456789012345678901234567890 40
> + 12345678901234567890123456789012345678901234567890 50
> +123456789012345678901234567890123456789012345678901234567890 60
> +
> diff --git a/test/unittest/funcs/strlen/tst.capped-sizw.d b/test/unittest/funcs/strlen/tst.capped-sizw.d
> new file mode 100644
> index 00000000..4e473236
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/tst.capped-sizw.d
> @@ -0,0 +1,25 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: strlen() obeys the maximum string size setting
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +#pragma D option strsize=5
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + exit(strlen("123456789") == 5 ? 0 : 1);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/strlen/tst.strlen1.d b/test/unittest/funcs/strlen/tst.empty.d
> similarity index 50%
> rename from test/unittest/strlen/tst.strlen1.d
> rename to test/unittest/funcs/strlen/tst.empty.d
> index aed5623c..ede41c36 100644
> --- a/test/unittest/strlen/tst.strlen1.d
> +++ b/test/unittest/funcs/strlen/tst.empty.d
> @@ -1,25 +1,24 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
> + * 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.
> */
>
> /*
> - * ASSERTION:
> - * Test the strlen() subroutine.
> + * ASSERTION: strlen("") is 0
> *
> * SECTION: Actions and Subroutines/strlen()
> - *
> */
>
> +#pragma D option quiet
> +
> BEGIN
> -/strlen("I like DTrace") == 13/
> {
> - correct = 1;
> + exit(strlen(""));
> }
>
> -BEGIN
> +ERROR
> {
> - exit(correct ? 0 : 1);
> + exit(1);
> }
> diff --git a/test/unittest/funcs/strlen/tst.null.d b/test/unittest/funcs/strlen/tst.null.d
> new file mode 100644
> index 00000000..d2bff0f6
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/tst.null.d
> @@ -0,0 +1,25 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * ASSERTION: Passing NULL (0) as argument to strlen() triggers an error.
> + *
> + * SECTION: Actions and Subroutines/strlen()
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + strlen(0);
> + exit(1);
> +}
> +
> +ERROR
> +{
> + exit(0);
> +}
> diff --git a/test/unittest/funcs/strlen/tst.null.r b/test/unittest/funcs/strlen/tst.null.r
> new file mode 100644
> index 00000000..ef424aed
> --- /dev/null
> +++ b/test/unittest/funcs/strlen/tst.null.r
> @@ -0,0 +1,3 @@
> +
> +-- @@stderr --
> +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1
More information about the DTrace-devel
mailing list