All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf bpf: 8 byte align bpil data
@ 2022-06-14  1:47 Ian Rogers
  2022-06-27 17:17 ` Ian Rogers
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Ian Rogers @ 2022-06-14  1:47 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-users,
	linux-kernel, netdev, bpf
  Cc: Stephane Eranian, Ian Rogers

bpil data is accessed assuming 64-bit alignment resulting in undefined
behavior as the data is just byte aligned. With an -fsanitize=undefined
build the following errors are observed:

$ sudo perf record -a sleep 1
util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
0x55f61084520f: note: pointer points here
 a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
             ^
util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
0x55f61084522f: note: pointer points here
 ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
             ^
util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
0x55f61084523f: note: pointer points here
 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00

Correct this by rouding up the data sizes and aligning the pointers.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/bpf-utils.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
index e271e05e51bc..80b1d2b3729b 100644
--- a/tools/perf/util/bpf-utils.c
+++ b/tools/perf/util/bpf-utils.c
@@ -149,11 +149,10 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
 		count = bpf_prog_info_read_offset_u32(&info, desc->count_offset);
 		size  = bpf_prog_info_read_offset_u32(&info, desc->size_offset);
 
-		data_len += count * size;
+		data_len += roundup(count * size, sizeof(__u64));
 	}
 
 	/* step 3: allocate continuous memory */
-	data_len = roundup(data_len, sizeof(__u64));
 	info_linear = malloc(sizeof(struct perf_bpil) + data_len);
 	if (!info_linear)
 		return ERR_PTR(-ENOMEM);
@@ -180,7 +179,7 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
 		bpf_prog_info_set_offset_u64(&info_linear->info,
 					     desc->array_offset,
 					     ptr_to_u64(ptr));
-		ptr += count * size;
+		ptr += roundup(count * size, sizeof(__u64));
 	}
 
 	/* step 5: call syscall again to get required arrays */
-- 
2.36.1.476.g0c4daa206d-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-14  1:47 [PATCH] perf bpf: 8 byte align bpil data Ian Rogers
@ 2022-06-27 17:17 ` Ian Rogers
  2022-06-27 20:56 ` Andrii Nakryiko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Ian Rogers @ 2022-06-27 17:17 UTC (permalink / raw)
  To: Dave Marchevsky
  Cc: Stephane Eranian, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, bpf, netdev, linux-kernel,
	linux-perf-users, Song Liu, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Yonghong Song, John Fastabend,
	KP Singh, Quentin Monnet

On Mon, Jun 13, 2022 at 6:47 PM Ian Rogers <irogers@google.com> wrote:
>
> bpil data is accessed assuming 64-bit alignment resulting in undefined
> behavior as the data is just byte aligned. With an -fsanitize=undefined
> build the following errors are observed:
>
> $ sudo perf record -a sleep 1
> util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> 0x55f61084520f: note: pointer points here
>  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
>              ^
> util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> 0x55f61084522f: note: pointer points here
>  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
>              ^
> util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> 0x55f61084523f: note: pointer points here
>  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
>
> Correct this by rouding up the data sizes and aligning the pointers.

Happy Monday, polite ping!

Thanks,
Ian

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/bpf-utils.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
> index e271e05e51bc..80b1d2b3729b 100644
> --- a/tools/perf/util/bpf-utils.c
> +++ b/tools/perf/util/bpf-utils.c
> @@ -149,11 +149,10 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>                 count = bpf_prog_info_read_offset_u32(&info, desc->count_offset);
>                 size  = bpf_prog_info_read_offset_u32(&info, desc->size_offset);
>
> -               data_len += count * size;
> +               data_len += roundup(count * size, sizeof(__u64));
>         }
>
>         /* step 3: allocate continuous memory */
> -       data_len = roundup(data_len, sizeof(__u64));
>         info_linear = malloc(sizeof(struct perf_bpil) + data_len);
>         if (!info_linear)
>                 return ERR_PTR(-ENOMEM);
> @@ -180,7 +179,7 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>                 bpf_prog_info_set_offset_u64(&info_linear->info,
>                                              desc->array_offset,
>                                              ptr_to_u64(ptr));
> -               ptr += count * size;
> +               ptr += roundup(count * size, sizeof(__u64));
>         }
>
>         /* step 5: call syscall again to get required arrays */
> --
> 2.36.1.476.g0c4daa206d-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-14  1:47 [PATCH] perf bpf: 8 byte align bpil data Ian Rogers
  2022-06-27 17:17 ` Ian Rogers
@ 2022-06-27 20:56 ` Andrii Nakryiko
  2022-06-28  8:14 ` Jiri Olsa
  2022-06-28  8:17 ` olsajiri
  3 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2022-06-27 20:56 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-use.,
	open list, Networking, bpf, Stephane Eranian

On Mon, Jun 13, 2022 at 6:47 PM Ian Rogers <irogers@google.com> wrote:
>
> bpil data is accessed assuming 64-bit alignment resulting in undefined
> behavior as the data is just byte aligned. With an -fsanitize=undefined
> build the following errors are observed:
>
> $ sudo perf record -a sleep 1
> util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> 0x55f61084520f: note: pointer points here
>  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
>              ^
> util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> 0x55f61084522f: note: pointer points here
>  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
>              ^
> util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> 0x55f61084523f: note: pointer points here
>  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
>
> Correct this by rouding up the data sizes and aligning the pointers.

typo: rounding

>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---

Makes sense.

Acked-by: Andrii Nakryiko <andrii@kernel.org>


>  tools/perf/util/bpf-utils.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
> index e271e05e51bc..80b1d2b3729b 100644
> --- a/tools/perf/util/bpf-utils.c
> +++ b/tools/perf/util/bpf-utils.c
> @@ -149,11 +149,10 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>                 count = bpf_prog_info_read_offset_u32(&info, desc->count_offset);
>                 size  = bpf_prog_info_read_offset_u32(&info, desc->size_offset);
>
> -               data_len += count * size;
> +               data_len += roundup(count * size, sizeof(__u64));
>         }
>
>         /* step 3: allocate continuous memory */
> -       data_len = roundup(data_len, sizeof(__u64));
>         info_linear = malloc(sizeof(struct perf_bpil) + data_len);
>         if (!info_linear)
>                 return ERR_PTR(-ENOMEM);
> @@ -180,7 +179,7 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>                 bpf_prog_info_set_offset_u64(&info_linear->info,
>                                              desc->array_offset,
>                                              ptr_to_u64(ptr));
> -               ptr += count * size;
> +               ptr += roundup(count * size, sizeof(__u64));
>         }
>
>         /* step 5: call syscall again to get required arrays */
> --
> 2.36.1.476.g0c4daa206d-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-14  1:47 [PATCH] perf bpf: 8 byte align bpil data Ian Rogers
  2022-06-27 17:17 ` Ian Rogers
  2022-06-27 20:56 ` Andrii Nakryiko
@ 2022-06-28  8:14 ` Jiri Olsa
  2022-06-28 15:11   ` Arnaldo Carvalho de Melo
  2022-06-28  8:17 ` olsajiri
  3 siblings, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2022-06-28  8:14 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-users,
	linux-kernel, netdev, bpf, Stephane Eranian

On Mon, Jun 13, 2022 at 06:47:14PM -0700, Ian Rogers wrote:
> bpil data is accessed assuming 64-bit alignment resulting in undefined
> behavior as the data is just byte aligned. With an -fsanitize=undefined
> build the following errors are observed:
> 
> $ sudo perf record -a sleep 1
> util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> 0x55f61084520f: note: pointer points here
>  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
>              ^
> util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> 0x55f61084522f: note: pointer points here
>  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
>              ^
> util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> 0x55f61084523f: note: pointer points here
>  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
> 
> Correct this by rouding up the data sizes and aligning the pointers.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/bpf-utils.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
> index e271e05e51bc..80b1d2b3729b 100644
> --- a/tools/perf/util/bpf-utils.c
> +++ b/tools/perf/util/bpf-utils.c
> @@ -149,11 +149,10 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>  		count = bpf_prog_info_read_offset_u32(&info, desc->count_offset);
>  		size  = bpf_prog_info_read_offset_u32(&info, desc->size_offset);
>  
> -		data_len += count * size;
> +		data_len += roundup(count * size, sizeof(__u64));
>  	}
>  
>  	/* step 3: allocate continuous memory */
> -	data_len = roundup(data_len, sizeof(__u64));
>  	info_linear = malloc(sizeof(struct perf_bpil) + data_len);
>  	if (!info_linear)
>  		return ERR_PTR(-ENOMEM);
> @@ -180,7 +179,7 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
>  		bpf_prog_info_set_offset_u64(&info_linear->info,
>  					     desc->array_offset,
>  					     ptr_to_u64(ptr));
> -		ptr += count * size;
> +		ptr += roundup(count * size, sizeof(__u64));

this one depends on info_linear->data being alligned(8), right?

should we make sure it's allways the case like in the patch
below, or it's superfluous?

thanks,
jirka


---
diff --git a/tools/perf/util/bpf-utils.h b/tools/perf/util/bpf-utils.h
index 86a5055cdfad..1aba76c44116 100644
--- a/tools/perf/util/bpf-utils.h
+++ b/tools/perf/util/bpf-utils.h
@@ -60,7 +60,7 @@ struct perf_bpil {
 	/* which arrays are included in data */
 	__u64			arrays;
 	struct bpf_prog_info	info;
-	__u8			data[];
+	__u8			data[] __attribute__((aligned(8)));
 };
 
 struct perf_bpil *

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-14  1:47 [PATCH] perf bpf: 8 byte align bpil data Ian Rogers
                   ` (2 preceding siblings ...)
  2022-06-28  8:14 ` Jiri Olsa
@ 2022-06-28  8:17 ` olsajiri
  2022-06-28 15:15   ` Ian Rogers
  3 siblings, 1 reply; 8+ messages in thread
From: olsajiri @ 2022-06-28  8:17 UTC (permalink / raw)
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-users,
	linux-kernel, netdev, bpf, Stephane Eranian

On Mon, Jun 13, 2022 at 06:47:14PM -0700, Ian Rogers wrote:
> bpil data is accessed assuming 64-bit alignment resulting in undefined
> behavior as the data is just byte aligned. With an -fsanitize=undefined
> build the following errors are observed:

I need to add -w to get the clean build with that, do you see that as well?

  $ make EXTRA_CFLAGS='-fsanitize=undefined -w'

> 
> $ sudo perf record -a sleep 1
> util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> 0x55f61084520f: note: pointer points here
>  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
>              ^
> util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> 0x55f61084522f: note: pointer points here
>  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
>              ^
> util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> 0x55f61084523f: note: pointer points here
>  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00


and I'm also getting another error in:

[root@krava perf]# ./perf record -a sleep 1
util/synthetic-events.c:1202:11: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
0x00000286f7ea: note: pointer points here
 20 00  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
              ^ 
util/synthetic-events.c:1203:18: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
0x00000286f7ea: note: pointer points here
 20 00  01 00 01 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
              ^ 
util/synthetic-events.c:1206:46: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
0x00000286f7ea: note: pointer points here
 20 00  01 00 01 00 08 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
              ^ 
/home/jolsa/kernel/linux-perf/tools/include/asm-generic/bitops/atomic.h:10:29: runtime error: load of misaligned address 0x00000286f7f2 for type 'long unsigned int', which requires 8 byte alignment
0x00000286f7f2: note: pointer points here
 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  51 00 00 00 00 00
              ^ 

are you going to address this one as well?


the reason for this one is that 'data' in struct perf_record_cpu_map_data
is not alligned(8), so that's why I raised the question in my other reply ;-)

I wonder we should mark all tools/lib/perf/include/perf/event.h types
as packed to prevent any compiler padding

thanks,
jirka

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-28  8:14 ` Jiri Olsa
@ 2022-06-28 15:11   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-06-28 15:11 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Dave Marchevsky,
	Quentin Monnet, linux-perf-users, linux-kernel, netdev, bpf,
	Stephane Eranian

Em Tue, Jun 28, 2022 at 10:14:52AM +0200, Jiri Olsa escreveu:
> On Mon, Jun 13, 2022 at 06:47:14PM -0700, Ian Rogers wrote:
> > bpil data is accessed assuming 64-bit alignment resulting in undefined
> > behavior as the data is just byte aligned. With an -fsanitize=undefined
> > build the following errors are observed:
> > 
> > $ sudo perf record -a sleep 1
> > util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> > 0x55f61084520f: note: pointer points here
> >  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
> >              ^
> > util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> > 0x55f61084522f: note: pointer points here
> >  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
> >              ^
> > util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> > 0x55f61084523f: note: pointer points here
> >  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
> > 
> > Correct this by rouding up the data sizes and aligning the pointers.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/bpf-utils.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
> > index e271e05e51bc..80b1d2b3729b 100644
> > --- a/tools/perf/util/bpf-utils.c
> > +++ b/tools/perf/util/bpf-utils.c
> > @@ -149,11 +149,10 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
> >  		count = bpf_prog_info_read_offset_u32(&info, desc->count_offset);
> >  		size  = bpf_prog_info_read_offset_u32(&info, desc->size_offset);
> >  
> > -		data_len += count * size;
> > +		data_len += roundup(count * size, sizeof(__u64));
> >  	}
> >  
> >  	/* step 3: allocate continuous memory */
> > -	data_len = roundup(data_len, sizeof(__u64));
> >  	info_linear = malloc(sizeof(struct perf_bpil) + data_len);
> >  	if (!info_linear)
> >  		return ERR_PTR(-ENOMEM);
> > @@ -180,7 +179,7 @@ get_bpf_prog_info_linear(int fd, __u64 arrays)
> >  		bpf_prog_info_set_offset_u64(&info_linear->info,
> >  					     desc->array_offset,
> >  					     ptr_to_u64(ptr));
> > -		ptr += count * size;
> > +		ptr += roundup(count * size, sizeof(__u64));
> 
> this one depends on info_linear->data being alligned(8), right?
> 
> should we make sure it's allways the case like in the patch
> below, or it's superfluous?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/bpf-utils.h b/tools/perf/util/bpf-utils.h
> index 86a5055cdfad..1aba76c44116 100644
> --- a/tools/perf/util/bpf-utils.h
> +++ b/tools/perf/util/bpf-utils.h
> @@ -60,7 +60,7 @@ struct perf_bpil {
>  	/* which arrays are included in data */
>  	__u64			arrays;
>  	struct bpf_prog_info	info;
> -	__u8			data[];
> +	__u8			data[] __attribute__((aligned(8)));
>  };
>  
>  struct perf_bpil *

⬢[acme@toolbox perf-urgent]$ pahole -C perf_bpil ~/bin/perf
struct perf_bpil {
	__u32                      info_len;             /*     0     4 */
	__u32                      data_len;             /*     4     4 */
	__u64                      arrays;               /*     8     8 */
	struct bpf_prog_info       info __attribute__((__aligned__(8))); /*    16   224 */

	/* XXX last struct has 4 bytes of padding */

	/* --- cacheline 3 boundary (192 bytes) was 48 bytes ago --- */
	__u8                       data[];               /*   240     0 */

	/* size: 240, cachelines: 4, members: 5 */
	/* paddings: 1, sum paddings: 4 */
	/* forced alignments: 1 */
	/* last cacheline: 48 bytes */
} __attribute__((__aligned__(8)));
⬢[acme@toolbox perf-urgent]$


Humm, lotsa explicit alignments already?

Looking at the sources:

struct perf_bpil {
        /* size of struct bpf_prog_info, when the tool is compiled */
        __u32                   info_len;
        /* total bytes allocated for data, round up to 8 bytes */
        __u32                   data_len;
        /* which arrays are included in data */
        __u64                   arrays;
        struct bpf_prog_info    info;
        __u8                    data[];
};

Interesting, where is pahole finding those aligned attributes? Ok
'struct bpf_prog_info' in tools/include/uapi/linux/bpf.h has aligned(8)
for the whole struct, so perf_bpil's info gets that.

sp that data right after 'info' is 8 byte alignedas
sizeof(bpf_prog_info) is a multiple of 8 bytes.

So I think I can apply the patch as-is and leave making sure data is
8-byte aligned for later.

Doing that now.

- Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-28  8:17 ` olsajiri
@ 2022-06-28 15:15   ` Ian Rogers
  2022-06-28 15:35     ` Jiri Olsa
  0 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2022-06-28 15:15 UTC (permalink / raw)
  To: olsajiri
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-users,
	linux-kernel, netdev, bpf, Stephane Eranian

On Tue, Jun 28, 2022 at 1:41 AM <olsajiri@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 06:47:14PM -0700, Ian Rogers wrote:
> > bpil data is accessed assuming 64-bit alignment resulting in undefined
> > behavior as the data is just byte aligned. With an -fsanitize=undefined
> > build the following errors are observed:
>
> I need to add -w to get the clean build with that, do you see that as well?
>
>   $ make EXTRA_CFLAGS='-fsanitize=undefined -w'

I don't recall needing this, but I was stacking fixes which may explain it.

> >
> > $ sudo perf record -a sleep 1
> > util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> > 0x55f61084520f: note: pointer points here
> >  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
> >              ^
> > util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> > 0x55f61084522f: note: pointer points here
> >  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
> >              ^
> > util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> > 0x55f61084523f: note: pointer points here
> >  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
>
>
> and I'm also getting another error in:
>
> [root@krava perf]# ./perf record -a sleep 1
> util/synthetic-events.c:1202:11: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> 0x00000286f7ea: note: pointer points here
>  20 00  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
>               ^
> util/synthetic-events.c:1203:18: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> 0x00000286f7ea: note: pointer points here
>  20 00  01 00 01 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
>               ^
> util/synthetic-events.c:1206:46: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> 0x00000286f7ea: note: pointer points here
>  20 00  01 00 01 00 08 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
>               ^
> /home/jolsa/kernel/linux-perf/tools/include/asm-generic/bitops/atomic.h:10:29: runtime error: load of misaligned address 0x00000286f7f2 for type 'long unsigned int', which requires 8 byte alignment
> 0x00000286f7f2: note: pointer points here
>  00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  51 00 00 00 00 00
>               ^
>
> are you going to address this one as well?
>
>
> the reason for this one is that 'data' in struct perf_record_cpu_map_data
> is not alligned(8), so that's why I raised the question in my other reply ;-)
>
> I wonder we should mark all tools/lib/perf/include/perf/event.h types
> as packed to prevent any compiler padding

I already sent out a fix and some improvements related to this:
https://lore.kernel.org/lkml/20220614143353.1559597-1-irogers@google.com/
Could you take a look?

I'm not sure about aligned and packed. I tried to minimize it in the
change above. The issue is that taking the address of a variable in a
packed struct results in an unaligned pointer. To address this in the
fix above I changed the functions to pass pointers to the whole
struct.

Thanks,
Ian

> thanks,
> jirka

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] perf bpf: 8 byte align bpil data
  2022-06-28 15:15   ` Ian Rogers
@ 2022-06-28 15:35     ` Jiri Olsa
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2022-06-28 15:35 UTC (permalink / raw)
  To: Ian Rogers
  Cc: olsajiri, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Dave Marchevsky, Quentin Monnet, linux-perf-users,
	linux-kernel, netdev, bpf, Stephane Eranian

On Tue, Jun 28, 2022 at 08:15:04AM -0700, Ian Rogers wrote:
> On Tue, Jun 28, 2022 at 1:41 AM <olsajiri@gmail.com> wrote:
> >
> > On Mon, Jun 13, 2022 at 06:47:14PM -0700, Ian Rogers wrote:
> > > bpil data is accessed assuming 64-bit alignment resulting in undefined
> > > behavior as the data is just byte aligned. With an -fsanitize=undefined
> > > build the following errors are observed:
> >
> > I need to add -w to get the clean build with that, do you see that as well?
> >
> >   $ make EXTRA_CFLAGS='-fsanitize=undefined -w'
> 
> I don't recall needing this, but I was stacking fixes which may explain it.
> 
> > >
> > > $ sudo perf record -a sleep 1
> > > util/bpf-event.c:310:22: runtime error: load of misaligned address 0x55f61084520f for type '__u64', which requires 8 byte alignment
> > > 0x55f61084520f: note: pointer points here
> > >  a8 fe ff ff 3c  51 d3 c0 ff ff ff ff 04  84 d3 c0 ff ff ff ff d8  aa d3 c0 ff ff ff ff a4  c0 d3 c0
> > >              ^
> > > util/bpf-event.c:311:20: runtime error: load of misaligned address 0x55f61084522f for type '__u32', which requires 4 byte alignment
> > > 0x55f61084522f: note: pointer points here
> > >  ff ff ff ff c7  17 00 00 f1 02 00 00 1f  04 00 00 58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00
> > >              ^
> > > util/bpf-event.c:198:33: runtime error: member access within misaligned address 0x55f61084523f for type 'const struct bpf_func_info', which requires 4 byte alignment
> > > 0x55f61084523f: note: pointer points here
> > >  58 04 00 00 00  00 00 00 0f 00 00 00 63  02 00 00 3b 00 00 00 ab  02 00 00 44 00 00 00 14  03 00 00
> >
> >
> > and I'm also getting another error in:
> >
> > [root@krava perf]# ./perf record -a sleep 1
> > util/synthetic-events.c:1202:11: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> > 0x00000286f7ea: note: pointer points here
> >  20 00  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
> >               ^
> > util/synthetic-events.c:1203:18: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> > 0x00000286f7ea: note: pointer points here
> >  20 00  01 00 01 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
> >               ^
> > util/synthetic-events.c:1206:46: runtime error: member access within misaligned address 0x00000286f7ea for type 'struct perf_record_record_cpu_map', which requires 8 byte alignment
> > 0x00000286f7ea: note: pointer points here
> >  20 00  01 00 01 00 08 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
> >               ^
> > /home/jolsa/kernel/linux-perf/tools/include/asm-generic/bitops/atomic.h:10:29: runtime error: load of misaligned address 0x00000286f7f2 for type 'long unsigned int', which requires 8 byte alignment
> > 0x00000286f7f2: note: pointer points here
> >  00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  51 00 00 00 00 00
> >               ^
> >
> > are you going to address this one as well?
> >
> >
> > the reason for this one is that 'data' in struct perf_record_cpu_map_data
> > is not alligned(8), so that's why I raised the question in my other reply ;-)
> >
> > I wonder we should mark all tools/lib/perf/include/perf/event.h types
> > as packed to prevent any compiler padding
> 
> I already sent out a fix and some improvements related to this:
> https://lore.kernel.org/lkml/20220614143353.1559597-1-irogers@google.com/
> Could you take a look?

ok, I overlooked that one

> 
> I'm not sure about aligned and packed. I tried to minimize it in the
> change above. The issue is that taking the address of a variable in a
> packed struct results in an unaligned pointer. To address this in the
> fix above I changed the functions to pass pointers to the whole
> struct.

ok, will check,

thanks,
jirka

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-06-28 15:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-14  1:47 [PATCH] perf bpf: 8 byte align bpil data Ian Rogers
2022-06-27 17:17 ` Ian Rogers
2022-06-27 20:56 ` Andrii Nakryiko
2022-06-28  8:14 ` Jiri Olsa
2022-06-28 15:11   ` Arnaldo Carvalho de Melo
2022-06-28  8:17 ` olsajiri
2022-06-28 15:15   ` Ian Rogers
2022-06-28 15:35     ` Jiri Olsa

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.