[RFC][PATCH 0/2] Perf misaligned address fixes

[RFC][PATCH 0/2] Perf misaligned address fixes

[Numfor Mbiziwo-Tiapo]

These patches are all errors found by running perf test with the
ubsan (undefined behavior sanitizer version of perf).

They are solutions to misaligned address errors caught by ubsan
but they would break compatibility between perf data files.

To build perf with ubsan run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

Perf will throw errors that have been fixed in these patches
that have not yet been merged:

https://lore.kernel.org/patchwork/patch/1104065/
https://lore.kernel.org/patchwork/patch/1104066/

Please feel free to leave comments. 

Numfor Mbiziwo-Tiapo (2):
  Fix event.c misaligned address error
  Fix evsel.c misaligned address errors

 tools/perf/util/event.h |  1 +
 tools/perf/util/evsel.c | 28 ++++++++++++++++------------
 2 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.22.0.657.g960e92d24f-goog

[RFC][PATCH 1/2] Fix event.c misaligned address error

[Numfor Mbiziwo-Tiapo]

The ubsan (undefined behavior sanitizer) build of perf throws an
error when the synthesize "Synthesize cpu map" function from
perf test is run.

This can be reproduced by running (from the tip directory):
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

(see cover letter for why perf may not build)

then running: tools/perf/perf test 44 -v

This bug occurs because the cpu_map_data__synthesize function in
event.c calls synthesize_mask, casting the 'data' variable
(of type cpu_map_data*) to a cpu_map_mask*. Since struct
cpu_map_data is 2 byte aligned and struct cpu_map_mask is 8 byte
aligned this causes memory alignment issues.

This is fixed by adding 6 bytes of padding to the struct cpu_map_data,
however, this will break compatibility between perf data files - a file
written by an old perf wouldn't work with a perf with this patch due
to event data alignment changing.

Comments?

Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/util/event.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index eb95f3384958..82eaf06c2604 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -433,6 +433,7 @@ struct cpu_map_mask {
 
 struct cpu_map_data {
 	u16	type;
+	u8 pad[6];
 	char	data[];
 };
 
-- 
2.22.0.657.g960e92d24f-goog

[RFC][PATCH 2/2] Fix evsel.c misaligned address errors

[Numfor Mbiziwo-Tiapo]

The ubsan (undefined behavior sanitizer) build of perf throws
misaligned address erros during 'Sample parsing function' in
perf test.

To reproduce, run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"

(see the cover letter for why perf may not build)

then run: tools/perf/perf test 26 -v

Most of the misaligned address errors come from improperly assigning
values to the u64 array in the 'perf_event__synthesize_sample'
function. These are easily fixed by changing the assignments
to use memcpy instead.

In the 'perf_evsel__parse_sample' function, the 'u64* array'
variable has varying numbers of bytes added to it depending on
which if statements it passes. Since this function is called
multiple times under different conditions, the 'array' variable
is sometimes misaligned by 4 bytes and sometimes not. This causes
issues when 'data->branch_stack' is later assigned to an element
in the array.

In the case that the array is misaligned we can add 4 bytes to the
array to realign it. This still causes an incorrect perf data file
(so the test still fails with the ubsan build) but it at least
gets rid of the error.

Comments?

Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/util/evsel.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbc0466db368..a1289fcbbb2d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2288,6 +2288,11 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 					  sizeof(struct branch_entry);
 
 		OVERFLOW_CHECK_u64(array);
+		if ((((u64)array) & 7) != 0)
+			array = (void *)array + sizeof(u32);
+
+		assert((((u64)array) & 7) == 0);
+		
 		data->branch_stack = (struct branch_stack *)array++;
 
 		if (data->branch_stack->nr > max_branch_nr)
@@ -2646,7 +2651,8 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type,
 
 	if (type & PERF_SAMPLE_REGS_USER) {
 		if (sample->user_regs.abi) {
-			*array++ = sample->user_regs.abi;
+			memcpy(array++, &sample->user_regs.abi,
+				sizeof(sample->user_regs.abi));
 			sz = hweight_long(sample->user_regs.mask) * sizeof(u64);
 			memcpy(array, sample->user_regs.regs, sz);
 			array = (void *)array + sz;
@@ -2657,32 +2663,31 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type,
 
 	if (type & PERF_SAMPLE_STACK_USER) {
 		sz = sample->user_stack.size;
-		*array++ = sz;
+		memcpy(array++, &sz, sizeof(sample->user_stack.size));
 		if (sz) {
 			memcpy(array, sample->user_stack.data, sz);
 			array = (void *)array + sz;
-			*array++ = sz;
+			memcpy(array++, &sz, sizeof(sz));
 		}
 	}
 
 	if (type & PERF_SAMPLE_WEIGHT) {
-		*array = sample->weight;
-		array++;
+		memcpy(array++, &sample->weight, sizeof(sample->weight));
 	}
 
 	if (type & PERF_SAMPLE_DATA_SRC) {
-		*array = sample->data_src;
-		array++;
+		memcpy(array++, &sample->data_src, sizeof(sample->data_src));
 	}
 
 	if (type & PERF_SAMPLE_TRANSACTION) {
-		*array = sample->transaction;
-		array++;
+		memcpy(array++, &sample->transaction,
+			sizeof(sample->transaction));
 	}
 
 	if (type & PERF_SAMPLE_REGS_INTR) {
 		if (sample->intr_regs.abi) {
-			*array++ = sample->intr_regs.abi;
+			memcpy(array++, &sample->intr_regs.abi,
+				sizeof(sample->intr_regs.abi));
 			sz = hweight_long(sample->intr_regs.mask) * sizeof(u64);
 			memcpy(array, sample->intr_regs.regs, sz);
 			array = (void *)array + sz;
@@ -2692,8 +2697,7 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type,
 	}
 
 	if (type & PERF_SAMPLE_PHYS_ADDR) {
-		*array = sample->phys_addr;
-		array++;
+		memcpy(array++, &sample->phys_addr, sizeof(sample->phys_addr));
 	}
 
 	return 0;
-- 
2.22.0.657.g960e92d24f-goog

Re: [RFC][PATCH 1/2] Fix event.c misaligned address error

[Ian Rogers]

An idea here is that, if this breaks backward compatibility, we
introduce an aligned variant and work to deprecate the unaligned
variant. I will look into making a patch set.

Thanks,
Ian

On Wed, Jul 24, 2019 at 3:50 PM Numfor Mbiziwo-Tiapo <nums@google.com> wrote:
>
> The ubsan (undefined behavior sanitizer) build of perf throws an
> error when the synthesize "Synthesize cpu map" function from
> perf test is run.
>
> This can be reproduced by running (from the tip directory):
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>
> (see cover letter for why perf may not build)
>
> then running: tools/perf/perf test 44 -v
>
> This bug occurs because the cpu_map_data__synthesize function in
> event.c calls synthesize_mask, casting the 'data' variable
> (of type cpu_map_data*) to a cpu_map_mask*. Since struct
> cpu_map_data is 2 byte aligned and struct cpu_map_mask is 8 byte
> aligned this causes memory alignment issues.
>
> This is fixed by adding 6 bytes of padding to the struct cpu_map_data,
> however, this will break compatibility between perf data files - a file
> written by an old perf wouldn't work with a perf with this patch due
> to event data alignment changing.
>
> Comments?
>
> Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/util/event.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index eb95f3384958..82eaf06c2604 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -433,6 +433,7 @@ struct cpu_map_mask {
>
>  struct cpu_map_data {
>         u16     type;
> +       u8 pad[6];
>         char    data[];
>  };
>
> --
> 2.22.0.657.g960e92d24f-goog
>

RE: [RFC][PATCH 1/2] Fix event.c misaligned address error

[David Laight]

From: Ian Rogers
> Sent: 11 September 2019 10:09
> On Wed, Jul 24, 2019 at 3:50 PM Numfor Mbiziwo-Tiapo <nums@google.com> wrote:
> >
> > The ubsan (undefined behavior sanitizer) build of perf throws an
> > error when the synthesize "Synthesize cpu map" function from
> > perf test is run.
> >
> > This can be reproduced by running (from the tip directory):
> > make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >
> > (see cover letter for why perf may not build)
> >
> > then running: tools/perf/perf test 44 -v
> >
> > This bug occurs because the cpu_map_data__synthesize function in
> > event.c calls synthesize_mask, casting the 'data' variable
> > (of type cpu_map_data*) to a cpu_map_mask*. Since struct
> > cpu_map_data is 2 byte aligned and struct cpu_map_mask is 8 byte
> > aligned this causes memory alignment issues.
> >
> > This is fixed by adding 6 bytes of padding to the struct cpu_map_data,
> > however, this will break compatibility between perf data files - a file
> > written by an old perf wouldn't work with a perf with this patch due
> > to event data alignment changing.
> >
> > Comments?
> >
> > Not-Quite-Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > ---
> >  tools/perf/util/event.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> > index eb95f3384958..82eaf06c2604 100644
> > --- a/tools/perf/util/event.h
> > +++ b/tools/perf/util/event.h
> > @@ -433,6 +433,7 @@ struct cpu_map_mask {
> >
> >  struct cpu_map_data {
> >         u16     type;
> > +       u8 pad[6];
> >         char    data[];
> >  };
> >
> > --
> > 2.22.0.657.g960e92d24f-goog
> >
> An idea here is that, if this breaks backward compatibility, we
> introduce an aligned variant and work to deprecate the unaligned
> variant. I will look into making a patch set.

Adding a pad[6] makes no difference.
You need to add aligened(8) to the structure itself as well.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)