On Sun, May 29, 2022 at 3:37 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Allows to specify expected and unexpected instruction sequences in
> test_verifier test cases. The instructions are requested from kernel
> after BPF program loading, thus allowing to check some of the
> transformations applied by BPF verifier.
>
> - `expected_insn` field specifies a sequence of instructions expected
> to be found in the program;
> - `unexpected_insn` field specifies a sequence of instructions that
> are not expected to be found in the program;
> - `INSN_OFF_MASK` and `INSN_IMM_MASK` values could be used to mask
> `off` and `imm` fields.
> - `SKIP_INSNS` could be used to specify that some instructions in the
> (un)expected pattern are not important (behavior similar to usage of
> `\t` in `errstr` field).
>
> The intended usage is as follows:
>
> {
> "inline simple bpf_loop call",
> .insns = {
> /* main */
> BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, BPF_REG_2,
> BPF_PSEUDO_FUNC, 0, 6),
> ...
> BPF_EXIT_INSN(),
> /* callback */
> BPF_ALU64_IMM(BPF_MOV, BPF_REG_0, 1),
> BPF_EXIT_INSN(),
> },
> .expected_insns = {
> BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> SKIP_INSN(),
nit: SKIP_INSNS(),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_CALL, 8, 1)
> },
> .unexpected_insns = {
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0,
> INSN_OFF_MASK, INSN_IMM_MASK),
> },
> .prog_type = BPF_PROG_TYPE_TRACEPOINT,
> .result = ACCEPT,
> .runs = 0,
> },
>
> Here it is expected that move of 1 to register 1 would remain in place
> and helper function call instruction would be replaced by a relative
> call instruction.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
[...]
>
> +static __u32 roundup_u32(__u32 number, __u32 divisor)
> +{
> + if (number % divisor == 0)
> + return number / divisor;
> + else
> + return number / divisor + 1;
> +}
Do we really need this roundup? If so, maybe give it a different name?
> +
> +static int get_xlated_program(int fd_prog, struct bpf_insn **buf, int *cnt)
> +{
> + struct bpf_prog_info info = {};
> + __u32 info_len = sizeof(info);
> + int err = 0;
> +
> + if (bpf_obj_get_info_by_fd(fd_prog, &info, &info_len)) {
> + err = errno;
> + perror("bpf_obj_get_info_by_fd failed");
> + goto out;
> + }
> +
> + __u32 xlated_prog_len = info.xlated_prog_len;
> + *cnt = roundup_u32(xlated_prog_len, sizeof(**buf));
> + *buf = calloc(*cnt, sizeof(**buf));
> + if (!buf) {
> + err = -ENOMEM;
> + perror("can't allocate xlated program buffer");
> + goto out;
> + }
> +
> + bzero(&info, sizeof(info));
> + info.xlated_prog_len = xlated_prog_len;
> + info.xlated_prog_insns = (__u64)*buf;
> +
> + if (bpf_obj_get_info_by_fd(fd_prog, &info, &info_len)) {
> + err = errno;
> + perror("second bpf_obj_get_info_by_fd failed");
> + goto out_free_buf;
> + }
> +
> + goto out;
Maybe just return 0 here, and ...
> +
> + out_free_buf:
> + free(*buf);
> + out:
... remove label "out:".
> + return err;
> +}
> +
Other than these, looks good to me.
Acked-by: Song Liu <songliubraving@xxxxxx>
