[PATCH] perf bpf: 8 byte align bpil data

Ian Rogers posted 1 patch 3 years, 10 months ago
tools/perf/util/bpf-utils.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] perf bpf: 8 byte align bpil data
Posted by Ian Rogers 3 years, 10 months ago
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
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by olsajiri@gmail.com 3 years, 10 months ago
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
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Ian Rogers 3 years, 10 months ago
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
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Jiri Olsa 3 years, 10 months ago
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
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Jiri Olsa 3 years, 10 months ago
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 *
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Arnaldo Carvalho de Melo 3 years, 10 months ago
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
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Andrii Nakryiko 3 years, 10 months ago
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
>
Re: [PATCH] perf bpf: 8 byte align bpil data
Posted by Ian Rogers 3 years, 10 months ago
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
>