Re: [PATCH] perf bpf: 8 byte align bpil data

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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