From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jiri Olsa <olsajiri@gmail.com>
Cc: Ian Rogers <irogers@google.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Namhyung Kim <namhyung@kernel.org>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
Dave Marchevsky <davemarchevsky@fb.com>,
Quentin Monnet <quentin@isovalent.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, bpf@vger.kernel.org,
Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH] perf bpf: 8 byte align bpil data
Date: Tue, 28 Jun 2022 12:11:47 -0300 [thread overview]
Message-ID: <YrsaM7q1KDKwfeLp@kernel.org> (raw)
In-Reply-To: <Yrq4fFtgcpwa2JUu@krava>
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
next prev parent reply other threads:[~2022-06-28 15:11 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2022-06-28 8:17 ` olsajiri
2022-06-28 15:15 ` Ian Rogers
2022-06-28 15:35 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YrsaM7q1KDKwfeLp@kernel.org \
--to=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davemarchevsky@fb.com \
--cc=eranian@google.com \
--cc=irogers@google.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olsajiri@gmail.com \
--cc=peterz@infradead.org \
--cc=quentin@isovalent.com \
--cc=songliubraving@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.