On Thu, Oct 19, 2023 at 6:41 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> From: Hou Tao <houtao1@xxxxxxxxxx>
>
> Test race between the release of map ref and bpf_timer_init():
> 1) create one thread to add array map with bpf_timer into array of
> arrays map repeatedly.
> 2) create another thread to call getpgid() and call bpf_timer_init()
> in the attached bpf program repeatedly.
> 3) synchronize these two threads through pthread barrier.
>
> It is a bit hard to trigger the kmemleak by only running the test. I
> managed to reproduce the kmemleak by injecting a delay between
> t->timer.function = bpf_timer_cb and timer->timer = t in
> bpf_timer_init().
I figured out that to trigger this issue reliably, I can insert
different delays using large bpf_loop() in allocation and release
paths. I have some extra code to filter out unwanted events. The
userspace program is similar. It just needs to try to call close(fd)
and syscall(SYS_getpgid) at the same time without delay. It is not a
stable test though due to the reference to the function.
SEC("tp/syscalls/sys_enter_close")
{
...
bpf_loop(1000000, &delay_loop, NULL, 0);
}
SEC("fexit/bpf_map_kmalloc_node")gmai
{
...
bpf_loop(2000000, &delay_loop, NULL, 0);
}
I can confirm that the v1 patch fixes memleak in v5.15. However, this
issue doesn't seem to affect net-next. At least since db559117828d
(bpf: Consolidate spin_lock, timer management into btf_record), the
leaked memory caused by the race would be freed in array_map_free().
>
> The following is the output of kmemleak after reproducing:
>
> unreferenced object 0xffff8881163d3780 (size 96):
> comm "test_progs", pid 539, jiffies 4295358164 (age 23.276s)
> hex dump (first 32 bytes):
> 80 37 3d 16 81 88 ff ff 00 00 00 00 00 00 00 00 .7=.............
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<00000000bbc3f059>] __kmem_cache_alloc_node+0x3b1/0x4a0
> [<00000000a24ddf4d>] __kmalloc_node+0x57/0x140
> [<000000004d577dbf>] bpf_map_kmalloc_node+0x5f/0x180
> [<00000000bd8428d3>] bpf_timer_init+0xf6/0x1b0
> [<0000000086d87323>] 0xffffffffc000c94e
> [<000000005a09e655>] trace_call_bpf+0xc5/0x1c0
> [<0000000051ab837b>] kprobe_perf_func+0x51/0x260
> [<000000000069bbd1>] kprobe_dispatcher+0x61/0x70
> [<000000007dceb75b>] kprobe_ftrace_handler+0x168/0x240
> [<00000000d8721bd7>] 0xffffffffc02010f7
> [<00000000e885b809>] __x64_sys_getpgid+0x1/0x20
> [<000000007be835d8>] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>
> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> ---
> .../bpf/prog_tests/timer_init_race.c | 138 ++++++++++++++++++
> .../selftests/bpf/progs/timer_init_race.c | 56 +++++++
> 2 files changed, 194 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/timer_init_race.c
> create mode 100644 tools/testing/selftests/bpf/progs/timer_init_race.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/timer_init_race.c b/tools/testing/selftests/bpf/prog_tests/timer_init_race.c
> new file mode 100644
> index 0000000000000..7bd57459e5048
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/timer_init_race.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2023. Huawei Technologies Co., Ltd */
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +#include "timer_init_race.skel.h"
> +
> +struct thread_ctx {
> + struct bpf_map_create_opts opts;
> + pthread_barrier_t barrier;
> + int outer_map_fd;
> + int start, abort;
> + int loop, err;
> +};
> +
> +static int wait_for_start_or_abort(struct thread_ctx *ctx)
> +{
> + while (!ctx->start && !ctx->abort)
> + usleep(1);
> + return ctx->abort ? -1 : 0;
> +}
> +
> +static void *close_map_fn(void *data)
> +{
> + struct thread_ctx *ctx = data;
> + int loop = ctx->loop, err = 0;
> +
> + if (wait_for_start_or_abort(ctx) < 0)
> + return NULL;
> +
> + while (loop-- > 0) {
> + int fd, zero = 0, i;
> + volatile int s = 0;
> +
> + fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, 4, sizeof(struct bpf_timer),
> + 1, &ctx->opts);
> + if (fd < 0) {
> + err |= 1;
> + pthread_barrier_wait(&ctx->barrier);
> + continue;
> + }
> +
> + if (bpf_map_update_elem(ctx->outer_map_fd, &zero, &fd, 0) < 0)
> + err |= 2;
> +
> + pthread_barrier_wait(&ctx->barrier);
> + /* let bpf_timer_init run first */
> + for (i = 0; i < 5000; i++)
> + s++;
> + close(fd);
> + }
> +
> + ctx->err = err;
> +
> + return NULL;
> +}
> +
> +static void *init_timer_fn(void *data)
> +{
> + struct thread_ctx *ctx = data;
> + int loop = ctx->loop;
> +
> + if (wait_for_start_or_abort(ctx) < 0)
> + return NULL;
> +
> + while (loop-- > 0) {
> + pthread_barrier_wait(&ctx->barrier);
> + syscall(SYS_getpgid);
> + }
> +
> + return NULL;
> +}
> +
> +void test_timer_init_race(void)
> +{
> + struct timer_init_race *skel;
> + struct thread_ctx ctx;
> + pthread_t tid[2];
> + struct btf *btf;
> + int err;
> +
> + skel = timer_init_race__open();
> + if (!ASSERT_OK_PTR(skel, "timer_init_race open"))
> + return;
> +
> + err = timer_init_race__load(skel);
> + if (!ASSERT_EQ(err, 0, "timer_init_race load"))
> + goto out;
> +
> + memset(&ctx, 0, sizeof(ctx));
> +
> + btf = bpf_object__btf(skel->obj);
> + if (!ASSERT_OK_PTR(btf, "timer_init_race btf"))
> + goto out;
> +
> + LIBBPF_OPTS_RESET(ctx.opts);
> + ctx.opts.btf_fd = bpf_object__btf_fd(skel->obj);
> + if (!ASSERT_GE((int)ctx.opts.btf_fd, 0, "btf_fd"))
> + goto out;
> + ctx.opts.btf_key_type_id = btf__find_by_name(btf, "int");
> + if (!ASSERT_GT(ctx.opts.btf_key_type_id, 0, "key_type_id"))
> + goto out;
> + ctx.opts.btf_value_type_id = btf__find_by_name_kind(btf, "inner_value", BTF_KIND_STRUCT);
> + if (!ASSERT_GT(ctx.opts.btf_value_type_id, 0, "value_type_id"))
> + goto out;
> +
> + err = timer_init_race__attach(skel);
> + if (!ASSERT_EQ(err, 0, "timer_init_race attach"))
> + goto out;
> +
> + skel->bss->tgid = getpid();
> +
> + pthread_barrier_init(&ctx.barrier, NULL, 2);
> + ctx.outer_map_fd = bpf_map__fd(skel->maps.outer_map);
> + ctx.loop = 8;
> +
> + err = pthread_create(&tid[0], NULL, close_map_fn, &ctx);
> + if (!ASSERT_OK(err, "close_thread"))
> + goto out;
> +
> + err = pthread_create(&tid[1], NULL, init_timer_fn, &ctx);
> + if (!ASSERT_OK(err, "init_thread")) {
> + ctx.abort = 1;
> + pthread_join(tid[0], NULL);
> + goto out;
> + }
> +
> + ctx.start = 1;
> + pthread_join(tid[0], NULL);
> + pthread_join(tid[1], NULL);
> +
> + ASSERT_EQ(ctx.err, 0, "error");
> + ASSERT_EQ(skel->bss->cnt, 8, "cnt");
> +out:
> + timer_init_race__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/timer_init_race.c b/tools/testing/selftests/bpf/progs/timer_init_race.c
> new file mode 100644
> index 0000000000000..ba67cb1786399
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/timer_init_race.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2023. Huawei Technologies Co., Ltd */
> +#include <linux/bpf.h>
> +#include <time.h>
> +#include <bpf/bpf_helpers.h>
> +
> +#include "bpf_misc.h"
> +
> +struct inner_value {
> + struct bpf_timer timer;
> +};
> +
> +struct inner_map_type {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __type(key, int);
> + __type(value, struct inner_value);
> + __uint(max_entries, 1);
> +} inner_map SEC(".maps");
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> + __type(key, int);
> + __type(value, int);
> + __uint(max_entries, 1);
> + __array(values, struct inner_map_type);
> +} outer_map SEC(".maps") = {
> + .values = {
> + [0] = &inner_map,
> + },
> +};
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int tgid = 0, cnt = 0;
> +
> +SEC("kprobe/" SYS_PREFIX "sys_getpgid")
> +int do_timer_init(void *ctx)
> +{
> + struct inner_map_type *map;
> + struct inner_value *value;
> + int zero = 0;
> +
> + if ((bpf_get_current_pid_tgid() >> 32) != tgid)
> + return 0;
> +
> + map = bpf_map_lookup_elem(&outer_map, &zero);
> + if (!map)
> + return 0;
> + value = bpf_map_lookup_elem(map, &zero);
> + if (!value)
> + return 0;
> + bpf_timer_init(&value->timer, map, CLOCK_MONOTONIC);
> + cnt++;
> +
> + return 0;
> +}
> --
> 2.29.2
>
