Re: [PATCH v5 bpf-next 05/15] selftests/bpf: add selftests for new insn_array map

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/10/02 06:16PM, Eduard Zingerman wrote:
> On Tue, 2025-09-30 at 12:51 +0000, Anton Protopopov wrote:
> 
> Overall lgtm, some code duplication worth removing, I think.
> 
> [...]
> 
> > +/*
> > + * Try to load a program with a map which points to outside of the program
> > + */
> > +static void check_out_of_bounds_index(void)
> > +{
> > +	struct bpf_insn insns[] = {
> > +		BPF_MOV64_IMM(BPF_REG_0, 4),
> > +		BPF_MOV64_IMM(BPF_REG_0, 3),
> > +		BPF_MOV64_IMM(BPF_REG_0, 2),
> > +		BPF_MOV64_IMM(BPF_REG_0, 1),
> > +		BPF_MOV64_IMM(BPF_REG_0, 0),
> > +		BPF_EXIT_INSN(),
> > +	};
> > +	int prog_fd, map_fd;
> > +	struct bpf_insn_array_value val = {};
> > +	int key;
> > +
> > +	map_fd = map_create(BPF_MAP_TYPE_INSN_ARRAY, 1);
> > +	if (!ASSERT_GE(map_fd, 0, "map_create"))
> > +		return;
> > +
> > +	key = 0;
> > +	val.xlated_off = ARRAY_SIZE(insns); /* too big */
> > +	if (!ASSERT_EQ(bpf_map_update_elem(map_fd, &key, &val, 0), 0, "bpf_map_update_elem"))
> > +		goto cleanup;
> > +
> > +	if (!ASSERT_EQ(bpf_map_freeze(map_fd), 0, "bpf_map_freeze"))
> > +		goto cleanup;
> > +
> > +	errno = 0;
> 
> Nit: errno is not used in the check below, hence there is no need to reset it.
>      (here and in other tests below)

Ok, will do.

> > +	prog_fd = prog_load(insns, ARRAY_SIZE(insns), &map_fd, 1);
> > +	if (!ASSERT_EQ(prog_fd, -EINVAL, "program should have been rejected (prog_fd != -EINVAL)")) {
> > +		close(prog_fd);
> > +		goto cleanup;
> > +	}
> > +
> > +cleanup:
> > +	close(map_fd);
> > +}
> 
> [...]
> 
> > +/*
> > + * Load a program with two patches (get jiffies, for simplicity). Add an
> > + * insn_array map pointing to every instruction. Check how it was changed
> > + * after the program load.
> > + */
> > +static void check_simple(void)
> > +{
> > +	struct bpf_insn insns[] = {
> > +		BPF_MOV64_IMM(BPF_REG_0, 2),
> > +		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_jiffies64),
> > +		BPF_MOV64_IMM(BPF_REG_0, 1),
> > +		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_jiffies64),
> > +		BPF_MOV64_IMM(BPF_REG_0, 0),
> > +		BPF_EXIT_INSN(),
> > +	};
> > +	int prog_fd = -1, map_fd;
> > +	__u32 map_in[] = {0, 1, 2, 3, 4, 5};
> > +	__u32 map_out[] = {0, 1, 4, 5, 8, 9};
> > +	struct bpf_insn_array_value val = {};
> > +	int i;
> > +
> > +	map_fd = map_create(BPF_MAP_TYPE_INSN_ARRAY, ARRAY_SIZE(insns));
> > +	if (!ASSERT_GE(map_fd, 0, "map_create"))
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(insns); i++) {
> > +		val.xlated_off = map_in[i];
> > +		if (!ASSERT_EQ(bpf_map_update_elem(map_fd, &i, &val, 0), 0,
> > +			       "bpf_map_update_elem"))
> > +			goto cleanup;
> > +	}
> > +
> > +	if (!ASSERT_EQ(bpf_map_freeze(map_fd), 0, "bpf_map_freeze"))
> > +		goto cleanup;
> > +
> > +	prog_fd = prog_load(insns, ARRAY_SIZE(insns), &map_fd, 1);
> > +	if (!ASSERT_GE(prog_fd, 0, "bpf(BPF_PROG_LOAD)"))
> > +		goto cleanup;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(insns); i++) {
> > +		if (!ASSERT_EQ(bpf_map_lookup_elem(map_fd, &i, &val), 0, "bpf_map_lookup_elem"))
> > +			goto cleanup;
> > +
> > +		ASSERT_EQ(val.xlated_off, map_out[i], "val should be equal map_out[i]");
> 
> Nit: maybe print `i`, `xlated_off` and `map_out[i]` here?

+1

> If this test fails, debugging it with -vvv would be inconvenient,
> as there is no way to see xlated program. Maybe extend load_prog()
> to check debug level and add capability to print xlated?
> See __xlated annotation implementation in selftests.

Ok, nice, will do.

Just in case, typically, when I write selftests I add smth like

    if (getenv("HANG") && *(getenv("HANG")))
        for (;;)
            pause();

before destroying skeleton, so I can examine what was loaded, if needed.

> 
> > +	}
> > +
> > +cleanup:
> > +	close(prog_fd);
> > +	close(map_fd);
> > +}
> > +
> > +/*
> > + * Verifier can delete code in two cases: nops & dead code. From insn
> > + * array's point of view, the two cases are the same, so test using
> > + * the simplest method: by loading some nops
> > + */
> > +static void check_deletions(void)
> > +{
> > +	struct bpf_insn insns[] = {
> > +		BPF_MOV64_IMM(BPF_REG_0, 2),
> > +		BPF_JMP_IMM(BPF_JA, 0, 0, 0), /* nop */
> > +		BPF_MOV64_IMM(BPF_REG_0, 1),
> > +		BPF_JMP_IMM(BPF_JA, 0, 0, 0), /* nop */
> > +		BPF_MOV64_IMM(BPF_REG_0, 0),
> > +		BPF_EXIT_INSN(),
> > +	};
> 
> Success test cases follow identical pattern, ultimately having 3 input
> parameters:
> - program
> - map_in
> - map_out
> 
> Would it make sense to write a generic utility function accepting
> exactly these three params and hiding the boilerplate?

Will do.

Are you ok with the contexts of tests? Any more test cases, maybe?

> > +	int prog_fd = -1, map_fd;
> > +	__u32 map_in[] = {0, 1, 2, 3, 4, 5};
> > +	__u32 map_out[] = {0, -1, 1, -1, 2, 3};
> > +	struct bpf_insn_array_value val = {};
> > +	int i;
> > +
> > +	map_fd = map_create(BPF_MAP_TYPE_INSN_ARRAY, ARRAY_SIZE(insns));
> > +	if (!ASSERT_GE(map_fd, 0, "map_create"))
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(insns); i++) {
> > +		val.xlated_off = map_in[i];
> > +		if (!ASSERT_EQ(bpf_map_update_elem(map_fd, &i, &val, 0), 0,
> > +			       "bpf_map_update_elem"))
> > +			goto cleanup;
> > +	}
> > +
> > +	if (!ASSERT_EQ(bpf_map_freeze(map_fd), 0, "bpf_map_freeze"))
> > +		goto cleanup;
> > +
> > +	prog_fd = prog_load(insns, ARRAY_SIZE(insns), &map_fd, 1);
> > +	if (!ASSERT_GE(prog_fd, 0, "bpf(BPF_PROG_LOAD)"))
> > +		goto cleanup;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(insns); i++) {
> > +		if (!ASSERT_EQ(bpf_map_lookup_elem(map_fd, &i, &val), 0, "bpf_map_lookup_elem"))
> > +			goto cleanup;
> > +
> > +		ASSERT_EQ(val.xlated_off, map_out[i], "val should be equal map_out[i]");
> > +	}
> > +
> > +cleanup:
> > +	close(prog_fd);
> > +	close(map_fd);
> > +}
> 
> [...]