Re: [PATCH bpf-next v1 1/3] selftests/bpf: Fix task_local_data data allocation size

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Amery Hung <ameryhung@gmail.com> writes:

> 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_datg_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 <ameryhung@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/task_local_data.h       | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> 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);
It looks like there is no point having Patch 1 separately, most of it is
overwritten in the Patch 2, I understand you want to have fix and
simplification separately, but is it really worth it for this selftest.
>  	if (!data_alloc) {
>  		err = -ENOMEM;
>  		goto out;
> -- 
> 2.52.0