From 7c8ca532a7413b9ecf533d870641e9cff333d685 Mon Sep 17 00:00:00 2001 From: Amery Hung Date: Tue, 31 Mar 2026 14:35:51 -0700 Subject: selftests/bpf: Fix task_local_data data allocation size Currently, when allocating memory for data, size of tld_data_u->start is not taken into account. This may cause OOB access. Fixed it by adding the non-flexible array part of tld_data_u. Besides, explicitly align tld_data_u->data to 8 bytes in case some fields are added before data in the future. It could break the assumption that every data field is 8 byte aligned and sizeof(tld_data_u) will no longer be equal to offsetof(struct tld_data_u, data), which we use interchangeably. Signed-off-by: Amery Hung Acked-by: Sun Jian Link: https://lore.kernel.org/r/20260331213555.1993883-2-ameryhung@gmail.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/prog_tests/task_local_data.h | 12 +++++++----- tools/testing/selftests/bpf/progs/task_local_data.bpf.h | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/bpf/prog_tests/task_local_data.h b/tools/testing/selftests/bpf/prog_tests/task_local_data.h index 7819f318b2fb..a52d8b549425 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_local_data.h +++ b/tools/testing/selftests/bpf/prog_tests/task_local_data.h @@ -90,7 +90,7 @@ typedef struct { struct tld_metadata { char name[TLD_NAME_LEN]; - _Atomic __u16 size; + _Atomic __u16 size; /* size of tld_data_u->data */ }; struct tld_meta_u { @@ -101,7 +101,7 @@ struct tld_meta_u { struct tld_data_u { __u64 start; /* offset of tld_data_u->data in a page */ - char data[]; + char data[] __attribute__((aligned(8))); }; struct tld_map_value { @@ -158,6 +158,7 @@ static int __tld_init_data_p(int map_fd) struct tld_data_u *data; void *data_alloc = NULL; int err, tid_fd = -1; + size_t size; tid_fd = syscall(SYS_pidfd_open, sys_gettid(), O_EXCL); if (tid_fd < 0) { @@ -173,9 +174,10 @@ static int __tld_init_data_p(int map_fd) * tld_meta_p->size = TLD_DYN_DATA_SIZE + * total size of TLDs defined via TLD_DEFINE_KEY() */ - data_alloc = (use_aligned_alloc || tld_meta_p->size * 2 >= TLD_PAGE_SIZE) ? - aligned_alloc(TLD_PAGE_SIZE, tld_meta_p->size) : - malloc(tld_meta_p->size * 2); + size = tld_meta_p->size + sizeof(struct tld_data_u); + data_alloc = (use_aligned_alloc || size * 2 >= TLD_PAGE_SIZE) ? + aligned_alloc(TLD_PAGE_SIZE, size) : + malloc(size * 2); if (!data_alloc) { err = -ENOMEM; goto out; diff --git a/tools/testing/selftests/bpf/progs/task_local_data.bpf.h b/tools/testing/selftests/bpf/progs/task_local_data.bpf.h index 8b6f7af43648..1f396711f487 100644 --- a/tools/testing/selftests/bpf/progs/task_local_data.bpf.h +++ b/tools/testing/selftests/bpf/progs/task_local_data.bpf.h @@ -87,7 +87,7 @@ struct tld_meta_u { struct tld_data_u { __u64 start; /* offset of tld_data_u->data in a page */ - char data[__PAGE_SIZE - sizeof(__u64)]; + char data[__PAGE_SIZE - sizeof(__u64)] __attribute__((aligned(8))); }; struct tld_map_value { -- cgit v1.2.3 From bb6d9f5cf1d407150658dc5c0dfa566ab2ec09ab Mon Sep 17 00:00:00 2001 From: Amery Hung Date: Tue, 31 Mar 2026 14:35:52 -0700 Subject: selftests/bpf: Simplify task_local_data memory allocation Simplify data allocation by always using aligned_alloc() and passing size_pot, size rounded up to the closest power of two to alignment. Currently, aligned_alloc(page_size, size) is only intended to be used with memory allocators that can fulfill the request without rounding size up to page_size to conserve memory. This is enabled by defining TLD_DATA_USE_ALIGNED_ALLOC. The reason to align to page_size is due to the limitation of UPTR where only a page can be pinned to the kernel. Otherwise, malloc(size * 2) is used to allocate memory for data. However, we don't need to call aligned_alloc(page_size, size) to get a contiguous memory of size bytes within a page. aligned_alloc(size_pot, ...) will also do the trick. Therefore, just use aligned_alloc(size_pot, ...) universally. As for the size argument, create a new option, TLD_DONT_ROUND_UP_DATA_SIZE, to specify not rounding up the size. This preserves the current TLD_DATA_USE_ALIGNED_ALLOC behavior, allowing memory allocators with low overhead aligned_alloc() to not waste memory. To enable this, users need to make sure it is not an undefined behavior for the memory allocator to have size not being an integral multiple of alignment. Compared to the current implementation, !TLD_DATA_USE_ALIGNED_ALLOC used to always waste size-byte of memory due to malloc(size * 2). Now the worst case becomes size - 1 and the best case is 0 when the size is already a power of two. Signed-off-by: Amery Hung Link: https://lore.kernel.org/r/20260331213555.1993883-3-ameryhung@gmail.com Signed-off-by: Alexei Starovoitov --- .../selftests/bpf/prog_tests/task_local_data.h | 60 ++++++++-------------- 1 file changed, 22 insertions(+), 38 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/bpf/prog_tests/task_local_data.h b/tools/testing/selftests/bpf/prog_tests/task_local_data.h index a52d8b549425..366a6739c086 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_local_data.h +++ b/tools/testing/selftests/bpf/prog_tests/task_local_data.h @@ -50,16 +50,13 @@ * TLD_MAX_DATA_CNT. * * - * TLD_DATA_USE_ALIGNED_ALLOC - Always use aligned_alloc() instead of malloc() + * TLD_DONT_ROUND_UP_DATA_SIZE - Don't round up memory size allocated for data if + * the memory allocator has low overhead aligned_alloc() implementation. * - * When allocating the memory for storing TLDs, we need to make sure there is a memory - * region of the X bytes within a page. This is due to the limit posed by UPTR: memory - * pinned to the kernel cannot exceed a page nor can it cross the page boundary. The - * library normally calls malloc(2*X) given X bytes of total TLDs, and only uses - * aligned_alloc(PAGE_SIZE, X) when X >= PAGE_SIZE / 2. This is to reduce memory wastage - * as not all memory allocator can use the exact amount of memory requested to fulfill - * aligned_alloc(). For example, some may round the size up to the alignment. Enable the - * option to always use aligned_alloc() if the implementation has low memory overhead. + * For some memory allocators, when calling aligned_alloc(alignment, size), size + * does not need to be an integral multiple of alignment and it can be fulfilled + * without using round_up(size, alignment) bytes of memory. Enable this option to + * reduce memory usage. */ #define TLD_PAGE_SIZE getpagesize() @@ -68,6 +65,8 @@ #define TLD_ROUND_MASK(x, y) ((__typeof__(x))((y) - 1)) #define TLD_ROUND_UP(x, y) ((((x) - 1) | TLD_ROUND_MASK(x, y)) + 1) +#define TLD_ROUND_UP_POWER_OF_TWO(x) (1UL << (sizeof(x) * 8 - __builtin_clzl(x - 1))) + #define TLD_READ_ONCE(x) (*(volatile typeof(x) *)&(x)) #ifndef TLD_DYN_DATA_SIZE @@ -111,7 +110,6 @@ struct tld_map_value { struct tld_meta_u * _Atomic tld_meta_p __attribute__((weak)); __thread struct tld_data_u *tld_data_p __attribute__((weak)); -__thread void *tld_data_alloc_p __attribute__((weak)); #ifdef TLD_FREE_DATA_ON_THREAD_EXIT pthread_key_t tld_pthread_key __attribute__((weak)); @@ -153,12 +151,10 @@ out: static int __tld_init_data_p(int map_fd) { - bool use_aligned_alloc = false; struct tld_map_value map_val; struct tld_data_u *data; - void *data_alloc = NULL; int err, tid_fd = -1; - size_t size; + size_t size, size_pot; tid_fd = syscall(SYS_pidfd_open, sys_gettid(), O_EXCL); if (tid_fd < 0) { @@ -166,48 +162,37 @@ static int __tld_init_data_p(int map_fd) goto out; } -#ifdef TLD_DATA_USE_ALIGNED_ALLOC - use_aligned_alloc = true; -#endif - /* * tld_meta_p->size = TLD_DYN_DATA_SIZE + * total size of TLDs defined via TLD_DEFINE_KEY() */ size = tld_meta_p->size + sizeof(struct tld_data_u); - data_alloc = (use_aligned_alloc || size * 2 >= TLD_PAGE_SIZE) ? - aligned_alloc(TLD_PAGE_SIZE, size) : - malloc(size * 2); - if (!data_alloc) { + size_pot = TLD_ROUND_UP_POWER_OF_TWO(size); +#ifdef TLD_DONT_ROUND_UP_DATA_SIZE + data = (struct tld_data_u *)aligned_alloc(size_pot, size); +#else + data = (struct tld_data_u *)aligned_alloc(size_pot, size_pot); +#endif + if (!data) { err = -ENOMEM; goto out; } /* * Always pass a page-aligned address to UPTR since the size of tld_map_value::data - * is a page in BTF. If data_alloc spans across two pages, use the page that contains large - * enough memory. + * is a page in BTF. */ - if (TLD_PAGE_SIZE - (~TLD_PAGE_MASK & (intptr_t)data_alloc) >= tld_meta_p->size) { - map_val.data = (void *)(TLD_PAGE_MASK & (intptr_t)data_alloc); - data = data_alloc; - data->start = (~TLD_PAGE_MASK & (intptr_t)data_alloc) + - offsetof(struct tld_data_u, data); - } else { - map_val.data = (void *)(TLD_ROUND_UP((intptr_t)data_alloc, TLD_PAGE_SIZE)); - data = (void *)(TLD_ROUND_UP((intptr_t)data_alloc, TLD_PAGE_SIZE)); - data->start = offsetof(struct tld_data_u, data); - } + map_val.data = (void *)(TLD_PAGE_MASK & (intptr_t)data); + data->start = (~TLD_PAGE_MASK & (intptr_t)data) + sizeof(struct tld_data_u); map_val.meta = TLD_READ_ONCE(tld_meta_p); err = bpf_map_update_elem(map_fd, &tid_fd, &map_val, 0); if (err) { - free(data_alloc); + free(data); goto out; } tld_data_p = data; - tld_data_alloc_p = data_alloc; #ifdef TLD_FREE_DATA_ON_THREAD_EXIT pthread_setspecific(tld_pthread_key, (void *)1); #endif @@ -375,9 +360,8 @@ static void *tld_get_data(int map_fd, tld_key_t key) __attribute__((unused)) static void tld_free(void) { - if (tld_data_alloc_p) { - free(tld_data_alloc_p); - tld_data_alloc_p = NULL; + if (tld_data_p) { + free(tld_data_p); tld_data_p = NULL; } } -- cgit v1.2.3 From 80aa8e9c64d0351f8bb215816e1b2277240c9e11 Mon Sep 17 00:00:00 2001 From: Amery Hung Date: Tue, 31 Mar 2026 14:35:53 -0700 Subject: selftests/bpf: Make sure TLD_DEFINE_KEY runs first Without specifying constructor priority of the hidden constructor function defined by TLD_DEFINE_KEY, __tld_create_key(..., dyn_data = false) may run after tld_get_data() called from other constructors. Threads calling tld_get_data() before __tld_create_key(..., dyn_data = false) will not allocate enough memory for all TLDs and later result in OOB access. Therefore, set it to the lowest value available to users. Note that lower means higher priority and 0-100 is reserved to the compiler. Acked-by: Mykyta Yatsenko Signed-off-by: Amery Hung Acked-by: Sun Jian Link: https://lore.kernel.org/r/20260331213555.1993883-4-ameryhung@gmail.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/prog_tests/task_local_data.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/bpf/prog_tests/task_local_data.h b/tools/testing/selftests/bpf/prog_tests/task_local_data.h index 366a6739c086..e242c455ddae 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_local_data.h +++ b/tools/testing/selftests/bpf/prog_tests/task_local_data.h @@ -278,7 +278,7 @@ retry: #define TLD_DEFINE_KEY(key, name, size) \ tld_key_t key; \ \ -__attribute__((constructor)) \ +__attribute__((constructor(101))) \ void __tld_define_key_##key(void) \ { \ key = __tld_create_key(name, size, false); \ -- cgit v1.2.3 From 0b481a6915ed649f12ae0a9625206781bfdcc705 Mon Sep 17 00:00:00 2001 From: Amery Hung Date: Tue, 31 Mar 2026 14:35:54 -0700 Subject: selftests/bpf: Remove TLD_READ_ONCE() in the user space header TLD_READ_ONCE() is redundant as the only reference passed to it is defined as _Atomic. The load is guaranteed to be atomic in C11 standard (6.2.6.1). Drop the macro. Signed-off-by: Amery Hung Acked-by: Sun Jian Link: https://lore.kernel.org/r/20260331213555.1993883-5-ameryhung@gmail.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/prog_tests/task_local_data.h | 8 +++----- tools/testing/selftests/bpf/prog_tests/test_task_local_data.c | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/bpf/prog_tests/task_local_data.h b/tools/testing/selftests/bpf/prog_tests/task_local_data.h index e242c455ddae..91f3486439bf 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_local_data.h +++ b/tools/testing/selftests/bpf/prog_tests/task_local_data.h @@ -67,8 +67,6 @@ #define TLD_ROUND_UP_POWER_OF_TWO(x) (1UL << (sizeof(x) * 8 - __builtin_clzl(x - 1))) -#define TLD_READ_ONCE(x) (*(volatile typeof(x) *)&(x)) - #ifndef TLD_DYN_DATA_SIZE #define TLD_DYN_DATA_SIZE 64 #endif @@ -184,7 +182,7 @@ static int __tld_init_data_p(int map_fd) */ map_val.data = (void *)(TLD_PAGE_MASK & (intptr_t)data); data->start = (~TLD_PAGE_MASK & (intptr_t)data) + sizeof(struct tld_data_u); - map_val.meta = TLD_READ_ONCE(tld_meta_p); + map_val.meta = tld_meta_p; err = bpf_map_update_elem(map_fd, &tid_fd, &map_val, 0); if (err) { @@ -207,7 +205,7 @@ static tld_key_t __tld_create_key(const char *name, size_t size, bool dyn_data) int err, i, sz, off = 0; __u16 cnt; - if (!TLD_READ_ONCE(tld_meta_p)) { + if (!tld_meta_p) { err = __tld_init_meta_p(); if (err) return (tld_key_t){(__s16)err}; @@ -338,7 +336,7 @@ static inline int tld_key_err_or_zero(tld_key_t key) __attribute__((unused)) static void *tld_get_data(int map_fd, tld_key_t key) { - if (!TLD_READ_ONCE(tld_meta_p)) + if (!tld_meta_p) return NULL; /* tld_data_p is allocated on the first invocation of tld_get_data() */ 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 9556ad3d986f..e219ff506b56 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 @@ -26,7 +26,7 @@ TLD_DEFINE_KEY(value0_key, "value0", sizeof(int)); */ static void reset_tld(void) { - if (TLD_READ_ONCE(tld_meta_p)) { + if (tld_meta_p) { /* Remove TLDs created by tld_create_key() */ tld_meta_p->cnt = 1; tld_meta_p->size = TLD_DYN_DATA_SIZE; -- cgit v1.2.3 From 63f5156a9c3e85ecfcc0127df6069cd7baa7eeb0 Mon Sep 17 00:00:00 2001 From: Amery Hung Date: Tue, 31 Mar 2026 14:35:55 -0700 Subject: selftests/bpf: Improve task local data documentation and fix potential memory leak If TLD_FREE_DATA_ON_THREAD_EXIT is not enabled in a translation unit that calls __tld_create_key() first, another translation unit that enables it will not get the auto cleanup feature as pthread key is only created once when allocation metadata. Fix it by always try to create the pthread key when __tld_create_key() is called. Also improve the documentation: - Discourage user from using different options in different translation units - Specify calling tld_free() before thread exit as undefined behavior Signed-off-by: Amery Hung Link: https://lore.kernel.org/r/20260331213555.1993883-6-ameryhung@gmail.com Signed-off-by: Alexei Starovoitov --- .../selftests/bpf/prog_tests/task_local_data.h | 25 ++++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) (limited to 'tools/testing') diff --git a/tools/testing/selftests/bpf/prog_tests/task_local_data.h b/tools/testing/selftests/bpf/prog_tests/task_local_data.h index 91f3486439bf..1e5c67c78ffb 100644 --- a/tools/testing/selftests/bpf/prog_tests/task_local_data.h +++ b/tools/testing/selftests/bpf/prog_tests/task_local_data.h @@ -22,14 +22,17 @@ /* * OPTIONS * - * Define the option before including the header + * Define the option before including the header. Using different options in + * different translation units is strongly discouraged. * * TLD_FREE_DATA_ON_THREAD_EXIT - Frees memory on thread exit automatically * * Thread-specific memory for storing TLD is allocated lazily on the first call to * tld_get_data(). The thread that calls it must also call tld_free() on thread exit * to prevent memory leak. Pthread will be included if the option is defined. A pthread - * key will be registered with a destructor that calls tld_free(). + * key will be registered with a destructor that calls tld_free(). Enabled only when + * the option is defined and TLD_DEFINE_KEY/tld_create_key() is called in the same + * translation unit. * * * TLD_DYN_DATA_SIZE - The maximum size of memory allocated for TLDs created dynamically @@ -47,7 +50,7 @@ * TLD_NAME_LEN - The maximum length of the name of a TLD (default: 62) * * Setting TLD_NAME_LEN will affect the maximum number of TLDs a process can store, - * TLD_MAX_DATA_CNT. + * TLD_MAX_DATA_CNT. Must be consistent with task_local_data.bpf.h. * * * TLD_DONT_ROUND_UP_DATA_SIZE - Don't round up memory size allocated for data if @@ -110,6 +113,7 @@ struct tld_meta_u * _Atomic tld_meta_p __attribute__((weak)); __thread struct tld_data_u *tld_data_p __attribute__((weak)); #ifdef TLD_FREE_DATA_ON_THREAD_EXIT +bool _Atomic tld_pthread_key_init __attribute__((weak)); pthread_key_t tld_pthread_key __attribute__((weak)); static void tld_free(void); @@ -140,9 +144,6 @@ static int __tld_init_meta_p(void) goto out; } -#ifdef TLD_FREE_DATA_ON_THREAD_EXIT - pthread_key_create(&tld_pthread_key, __tld_thread_exit_handler); -#endif out: return err; } @@ -203,6 +204,7 @@ out: static tld_key_t __tld_create_key(const char *name, size_t size, bool dyn_data) { int err, i, sz, off = 0; + bool uninit = false; __u16 cnt; if (!tld_meta_p) { @@ -211,6 +213,14 @@ static tld_key_t __tld_create_key(const char *name, size_t size, bool dyn_data) return (tld_key_t){(__s16)err}; } +#ifdef TLD_FREE_DATA_ON_THREAD_EXIT + if (atomic_compare_exchange_strong(&tld_pthread_key_init, &uninit, true)) { + err = pthread_key_create(&tld_pthread_key, __tld_thread_exit_handler); + if (err) + return (tld_key_t){(__s16)err}; + } +#endif + for (i = 0; i < (int)TLD_MAX_DATA_CNT; i++) { retry: cnt = atomic_load(&tld_meta_p->cnt); @@ -353,7 +363,8 @@ static void *tld_get_data(int map_fd, tld_key_t key) * * Users must call tld_free() on thread exit to prevent memory leak. Alternatively, * define TLD_FREE_DATA_ON_THREAD_EXIT and a thread exit handler will be registered - * to free the memory automatically. + * to free the memory automatically. Calling tld_free() before thread exit is + * undefined behavior, which may lead to null-pointer dereference. */ __attribute__((unused)) static void tld_free(void) -- cgit v1.2.3