| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1483fad6-709a-50f5-4b8e-358ad2848dfe@fb.com>
Date: Wed, 28 Jul 2021 19:56:38 -0700
From: Yonghong Song <yhs@...com>
To: Johan Almbladh <johan.almbladh@...finetworks.com>,
<ast@...nel.org>, <daniel@...earbox.net>, <andrii@...nel.org>
CC: <kafai@...com>, <songliubraving@...com>,
<john.fastabend@...il.com>, <kpsingh@...nel.org>,
<Tony.Ambardar@...il.com>, <netdev@...r.kernel.org>,
<bpf@...r.kernel.org>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH 14/14] bpf/tests: Add tail call test suite
On 7/28/21 10:05 AM, Johan Almbladh wrote:
> While BPF_CALL instructions were tested implicitly by the cBPF-to-eBPF
> translation, there has not been any tests for BPF_TAIL_CALL instructions.
> The new test suite includes tests for tail call chaining, tail call count
> tracking and error paths. It is mainly intended for JIT development and
> testing.
>
> Signed-off-by: Johan Almbladh <johan.almbladh@...finetworks.com>
> Reported-by: kernel test robot <lkp@...el.com>
The above Reported-by tag can be removed. This patch itself is not
about fixing an issue reported by kernel test robot...
The patch looks good to me except a few minor comments below.
Acked-by: Yonghong Song <yhs@...com>
> ---
> lib/test_bpf.c | 249 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 249 insertions(+)
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index af5758151d0a..222d454b2ed4 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -8981,8 +8981,249 @@ static __init int test_bpf(void)
> return err_cnt ? -EINVAL : 0;
> }
>
> +struct tail_call_test {
> + const char *descr;
> + struct bpf_insn insns[MAX_INSNS];
> + int result;
> + int stack_depth;
> +};
> +
> +/*
> + * Magic marker used in test snippets for tail calls below.
> + * BPF_LD/MOV to R2 and R2 with this immediate value is replaced
> + * with the proper values by the test runner.
> + */
> +#define TAIL_CALL_MARKER 0x7a11ca11
> +
> +/* Special offset to indicate a NULL call target */
> +#define TAIL_CALL_NULL 0x7fff
> +
> +#define TAIL_CALL(offset) \
> + BPF_LD_IMM64(R2, TAIL_CALL_MARKER), \
> + BPF_RAW_INSN(BPF_ALU | BPF_MOV | BPF_K, R3, 0, \
> + offset, TAIL_CALL_MARKER), \
> + BPF_JMP_IMM(BPF_TAIL_CALL, 0, 0, 0)
> +
> +/*
> + * Tail call tests. Each test case may call any other test in the table,
> + * including itself, specified as a relative index offset from the calling
> + * test. The index TAIL_CALL_NULL can be used to specify a NULL target
> + * function to test the JIT error path.
> + */
> +static struct tail_call_test tail_call_tests[] = {
> + {
> + "Tail call leaf",
> + .insns = {
> + BPF_ALU64_REG(BPF_MOV, R0, R1),
> + BPF_ALU64_IMM(BPF_ADD, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 1,
> + },
> + {
> + "Tail call 2",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, R1, 2),
> + TAIL_CALL(-1),
> + BPF_ALU64_IMM(BPF_MOV, R0, -1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 3,
> + },
> + {
> + "Tail call 3",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, R1, 3),
> + TAIL_CALL(-1),
> + BPF_ALU64_IMM(BPF_MOV, R0, -1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 6,
> + },
> + {
> + "Tail call 4",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, R1, 4),
> + TAIL_CALL(-1),
> + BPF_ALU64_IMM(BPF_MOV, R0, -1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 10,
> + },
> + {
> + "Tail call error path, max count reached",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, R1, 1),
> + BPF_ALU64_REG(BPF_MOV, R0, R1),
> + TAIL_CALL(0),
> + BPF_EXIT_INSN(),
> + },
> + .result = MAX_TAIL_CALL_CNT + 1,
> + },
> + {
> + "Tail call error path, NULL target",
> + .insns = {
> + BPF_ALU64_IMM(BPF_MOV, R0, -1),
> + TAIL_CALL(TAIL_CALL_NULL),
> + BPF_ALU64_IMM(BPF_MOV, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 1,
> + },
> + {
> + /* Must be the last test */
> + "Tail call error path, index out of range",
> + .insns = {
> + BPF_ALU64_IMM(BPF_MOV, R0, -1),
> + TAIL_CALL(1), /* Index out of range */
> + BPF_ALU64_IMM(BPF_MOV, R0, 1),
> + BPF_EXIT_INSN(),
> + },
> + .result = 1,
> + },
> +};
> +
> +static void __init destroy_tail_call_tests(struct bpf_array *progs)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(tail_call_tests); i++)
> + if (progs->ptrs[i])
> + bpf_prog_free(progs->ptrs[i]);
> + kfree(progs);
> +}
> +
> +static __init int prepare_tail_call_tests(struct bpf_array **pprogs)
> +{
> + struct bpf_array *progs;
> + int ntests = ARRAY_SIZE(tail_call_tests);
> + int which, err;
reverse christmas tree?
> +
> + /* Allocate the table of programs to be used for tall calls */
> + progs = kzalloc(sizeof(*progs) + (ntests + 1) * sizeof(progs->ptrs[0]),
> + GFP_KERNEL);
> + if (!progs)
> + goto out_nomem;
> +
> + /* Create all eBPF programs and populate the table */
> + for (which = 0; which < ntests; which++) {
> + struct tail_call_test *test = &tail_call_tests[which];
> + struct bpf_prog *fp;
> + int len, i;
> +
> + /* Compute the number of program instructions */
> + for (len = 0; len < MAX_INSNS; len++) {
> + struct bpf_insn *insn = &test->insns[len];
> +
> + if (len < MAX_INSNS - 1 &&
> + insn->code == (BPF_LD | BPF_DW | BPF_IMM))
> + len++;
> + if (insn->code == 0)
> + break;
> + }
> +
> + /* Allocate and initialize the program */
> + fp = bpf_prog_alloc(bpf_prog_size(len), 0);
> + if (!fp)
> + goto out_nomem;
> +
> + fp->len = len;
> + fp->type = BPF_PROG_TYPE_SOCKET_FILTER;
> + fp->aux->stack_depth = test->stack_depth;
> + memcpy(fp->insnsi, test->insns, len * sizeof(struct bpf_insn));
> +
> + /* Relocate runtime tail call offsets and addresses */
> + for (i = 0; i < len; i++) {
> + struct bpf_insn *insn = &fp->insnsi[i];
> + int target;
> +
> + if (insn->imm != TAIL_CALL_MARKER)
> + continue;
> +
> + switch (insn->code) {
> + case BPF_LD | BPF_DW | BPF_IMM:
> + if (insn->dst_reg == R2) {
Looks like the above condition is not needed. It is always true.
> + insn[0].imm = (u32)(long)progs;
> + insn[1].imm = ((u64)(long)progs) >> 32;
> + }
> + break;
> +
> + case BPF_ALU | BPF_MOV | BPF_K:
> + case BPF_ALU64 | BPF_MOV | BPF_K:
case BPF_ALU64 | BPF_MOV | BPF_K is not needed.
> + if (insn->off == TAIL_CALL_NULL)
> + target = ntests;
> + else
> + target = which + insn->off;
> + if (insn->dst_reg == R3)
the same here, insn->dst_reg == R3 is not needed. It is always true.
I suggest to set insn->off = 0. Otherwise, it is an illegal insn.
We won't issue here because we didn't invoke verifier. It is still
good to make the insn legel.
> + insn->imm = target;
> + break;
> + }
> + }
> +
> + fp = bpf_prog_select_runtime(fp, &err);
> + if (err)
> + goto out_err;
> +
> + progs->ptrs[which] = fp;
> + }
> +
> + /* The last entry contains a NULL program pointer */
> + progs->map.max_entries = ntests + 1;
> + *pprogs = progs;
> + return 0;
> +
> +out_nomem:
> + err = -ENOMEM;
> +
> +out_err:
> + if (progs)
> + destroy_tail_call_tests(progs);
> + return err;
> +}
> +
[...]
Powered by blists - more mailing lists