Re: [PATCH bpf v2 3/3] selftests/bpf: Add test for prealloc_lru_pop bug

[Yonghong Song <yhs@fb.com>]

On 8/9/22 7:06 AM, Kumar Kartikeya Dwivedi wrote:
> Add a regression test to check against invalid check_and_init_map_value
> call inside prealloc_lru_pop.
> 
> To actually observe a kind of problem this can cause, set debug to 1
> when running the test locally without the fix. Then, just observe the
> refcount which keeps increasing on each run of the test. With timers or
> spin locks, it would cause unpredictable results when racing.
> 
> ...
> 
> bash-5.1# ./test_progs -t lru_bug
>        test_progs-192     [000] d..21   354.838821: bpf_trace_printk: ref: 4
>        test_progs-192     [000] d..21   354.842824: bpf_trace_printk: ref: 5
> bash-5.1# ./test_progs -t lru_bug
>        test_progs-193     [000] d..21   356.722813: bpf_trace_printk: ref: 5
>        test_progs-193     [000] d..21   356.727071: bpf_trace_printk: ref: 6
> 
> ... and so on.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Ack with a minor nit below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../selftests/bpf/prog_tests/lru_bug.c        | 19 ++++++
>   tools/testing/selftests/bpf/progs/lru_bug.c   | 67 +++++++++++++++++++
>   2 files changed, 86 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/lru_bug.c
>   create mode 100644 tools/testing/selftests/bpf/progs/lru_bug.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/lru_bug.c b/tools/testing/selftests/bpf/prog_tests/lru_bug.c
> new file mode 100644
> index 000000000000..3bcb5bc62d5a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lru_bug.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +
> +#include "lru_bug.skel.h"
> +
> +void serial_test_lru_bug(void)
> +{
> +	struct lru_bug *skel;
> +	int ret;
> +
> +	skel = lru_bug__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "lru_bug__open_and_load"))
> +		return;
> +	ret = lru_bug__attach(skel);
> +	if (!ASSERT_OK(ret, "lru_bug__attach"))
> +		return;

If not ASSERT_OK, should go to lru_bug__destroy(skel).

> +	usleep(1);
> +	ASSERT_OK(skel->data->result, "prealloc_lru_pop doesn't call check_and_init_map_value");

Missing
	lru_bug__destroy(skel);
.

> +}
[...]