Message-ID: <CABFh=a75g_EUwEq_3PA+K4O6CPR9HnCq28xU7XY=-VcBknB5DA@mail.gmail.com>
Date: Mon, 28 Jul 2025 13:59:56 -0400
From: Emil Tsalapatis <linux-lists@...alapatis.com>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, tj@...nel.org, memxor@...il.com, 
	martin.lau@...nel.org, kernel-team@...a.com
Subject: Re: [PATCH bpf-next v6 3/3] selftests/bpf: Test concurrent task local
 data key creation

On Thu, Jul 17, 2025 at 12:49 PM Amery Hung <ameryhung@...il.com> wrote:
>
> Test thread-safety of tld_create_key(). Since tld_create_key() does
> not rely on locks but memory barriers and atomic operations to protect
> the shared metadata, the thread-safety of the function is non-trivial.
> Make sure concurrent tld_key_create(), both valid and invalid, can not
> race and corrupt metatada, which may leads to TLDs not being thread-
> specific or duplicate TLDs with the same name.
>
> Signed-off-by: Amery Hung <ameryhung@...il.com>

Reviewed-by: Emil Tsalapatis <emil@...alapatis.com>

> ---
>  .../bpf/prog_tests/test_task_local_data.c     | 105 ++++++++++++++++++
>  1 file changed, 105 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c b/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
> index fde4a030ab42..1d3ccb98b5db 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_task_local_data.c
> @@ -185,8 +185,113 @@ static void test_task_local_data_basic(void)
>         test_task_local_data__destroy(skel);
>  }
>
> +#define TEST_RACE_THREAD_NUM (TLD_MAX_DATA_CNT - 3)
> +
> +void *test_task_local_data_race_thread(void *arg)
> +{
> +       int err = 0, id = (intptr_t)arg;
> +       char key_name[32];
> +       tld_key_t key;
> +
> +       key = tld_create_key("value_not_exist", TLD_PAGE_SIZE + 1);
> +       if (tld_key_err_or_zero(key) != -E2BIG) {
> +               err = 1;
> +               goto out;
> +       }
> +
> +       /* Only one thread will succeed in creating value1 */
> +       key = tld_create_key("value1", sizeof(int));
> +       if (!tld_key_is_err(key))
> +               tld_keys[1] = key;
> +
> +       /* Only one thread will succeed in creating value2 */
> +       key = tld_create_key("value2", sizeof(struct test_tld_struct));
> +       if (!tld_key_is_err(key))
> +               tld_keys[2] = key;
> +
> +       snprintf(key_name, 32, "thread_%d", id);
> +       tld_keys[id] = tld_create_key(key_name, sizeof(int));
> +       if (tld_key_is_err(tld_keys[id]))
> +               err = 2;
> +out:
> +       return (void *)(intptr_t)err;
> +}
> +
> +static void test_task_local_data_race(void)
> +{
> +       LIBBPF_OPTS(bpf_test_run_opts, opts);
> +       pthread_t thread[TEST_RACE_THREAD_NUM];
> +       struct test_task_local_data *skel;
> +       int fd, i, j, err, *data;
> +       void *ret = NULL;
> +
> +       skel = test_task_local_data__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> +               return;
> +
> +       tld_keys = calloc(TLD_MAX_DATA_CNT, sizeof(tld_key_t));
> +       if (!ASSERT_OK_PTR(tld_keys, "calloc tld_keys"))
> +               goto out;
> +
> +       fd = bpf_map__fd(skel->maps.tld_data_map);
> +
> +       ASSERT_FALSE(tld_key_is_err(value0_key), "TLD_DEFINE_KEY");
> +       tld_keys[0] = value0_key;
> +
> +       for (j = 0; j < 100; j++) {
> +               reset_tld();
> +
> +               for (i = 0; i < TEST_RACE_THREAD_NUM; i++) {
> +                       /*
> +                        * Try to make tld_create_key() race with each other. Call
> +                        * tld_create_key(), both valid and invalid, from different threads.
> +                        */
> +                       err = pthread_create(&thread[i], NULL, test_task_local_data_race_thread,
> +                                            (void *)(intptr_t)(i + 3));
> +                       if (CHECK_FAIL(err))
> +                               break;
> +               }
> +
> +               /* Wait for all tld_create_key() to return */
> +               for (i = 0; i < TEST_RACE_THREAD_NUM; i++) {
> +                       pthread_join(thread[i], &ret);
> +                       if (CHECK_FAIL(ret))
> +                               break;
> +               }
> +
> +               /* Write a unique number to each TLD */
> +               for (i = 0; i < TLD_MAX_DATA_CNT; i++) {
> +                       data = tld_get_data(fd, tld_keys[i]);
> +                       if (CHECK_FAIL(!data))
> +                               break;
> +                       *data = i;
> +               }
> +
> +               /* Read TLDs and check the value to see if any address collides with another */
> +               for (i = 0; i < TLD_MAX_DATA_CNT; i++) {
> +                       data = tld_get_data(fd, tld_keys[i]);
> +                       if (CHECK_FAIL(*data != i))
> +                               break;
> +               }
> +
> +               /* Run task_main to make sure no invalid TLDs are added */
> +               err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.task_main), &opts);
> +               ASSERT_OK(err, "run task_main");
> +               ASSERT_OK(opts.retval, "task_main retval");
> +       }
> +out:
> +       if (tld_keys) {
> +               free(tld_keys);
> +               tld_keys = NULL;
> +       }
> +       tld_free();
> +       test_task_local_data__destroy(skel);
> +}
> +
>  void test_task_local_data(void)
>  {
>         if (test__start_subtest("task_local_data_basic"))
>                 test_task_local_data_basic();
> +       if (test__start_subtest("task_local_data_race"))
> +               test_task_local_data_race();
>  }
> --
> 2.47.1
>
>

Powered by blists - more mailing lists