* [RFC][PATCH 0/2] Perf misaligned address fixes @ 2019-07-24 22:49 Numfor Mbiziwo-Tiapo 2019-07-24 22:49 ` [RFC][PATCH 1/2] Fix event.c misaligned address error Numfor Mbiziwo-Tiapo 2019-07-24 22:49 ` [RFC][PATCH 2/2] Fix evsel.c misaligned address errors Numfor Mbiziwo-Tiapo 0 siblings, 2 replies; 5+ messages in thread From: Numfor Mbiziwo-Tiapo @ 2019-07-24 22:49 UTC (permalink / raw) To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, songliubraving, mbd Cc: linux-kernel, irogers, eranian, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC][PATCH 1/2] Fix event.c misaligned address error 2019-07-24 22:49 [RFC][PATCH 0/2] Perf misaligned address fixes Numfor Mbiziwo-Tiapo @ 2019-07-24 22:49 ` Numfor Mbiziwo-Tiapo 2019-09-11 9:08 ` Ian Rogers 2019-07-24 22:49 ` [RFC][PATCH 2/2] Fix evsel.c misaligned address errors Numfor Mbiziwo-Tiapo 1 sibling, 1 reply; 5+ messages in thread From: Numfor Mbiziwo-Tiapo @ 2019-07-24 22:49 UTC (permalink / raw) To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, songliubraving, mbd Cc: linux-kernel, irogers, eranian, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC][PATCH 1/2] Fix event.c misaligned address error 2019-07-24 22:49 ` [RFC][PATCH 1/2] Fix event.c misaligned address error Numfor Mbiziwo-Tiapo @ 2019-09-11 9:08 ` Ian Rogers 2019-09-12 8:50 ` David Laight 0 siblings, 1 reply; 5+ messages in thread From: Ian Rogers @ 2019-09-11 9:08 UTC (permalink / raw) To: Numfor Mbiziwo-Tiapo Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Song Liu, mbd, LKML, Stephane Eranian 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [RFC][PATCH 1/2] Fix event.c misaligned address error 2019-09-11 9:08 ` Ian Rogers @ 2019-09-12 8:50 ` David Laight 0 siblings, 0 replies; 5+ messages in thread From: David Laight @ 2019-09-12 8:50 UTC (permalink / raw) To: 'Ian Rogers', Numfor Mbiziwo-Tiapo Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Song Liu, mbd, LKML, Stephane Eranian 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC][PATCH 2/2] Fix evsel.c misaligned address errors 2019-07-24 22:49 [RFC][PATCH 0/2] Perf misaligned address fixes Numfor Mbiziwo-Tiapo 2019-07-24 22:49 ` [RFC][PATCH 1/2] Fix event.c misaligned address error Numfor Mbiziwo-Tiapo @ 2019-07-24 22:49 ` Numfor Mbiziwo-Tiapo 1 sibling, 0 replies; 5+ messages in thread From: Numfor Mbiziwo-Tiapo @ 2019-07-24 22:49 UTC (permalink / raw) To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung, songliubraving, mbd Cc: linux-kernel, irogers, eranian, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-12 8:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-24 22:49 [RFC][PATCH 0/2] Perf misaligned address fixes Numfor Mbiziwo-Tiapo 2019-07-24 22:49 ` [RFC][PATCH 1/2] Fix event.c misaligned address error Numfor Mbiziwo-Tiapo 2019-09-11 9:08 ` Ian Rogers 2019-09-12 8:50 ` David Laight 2019-07-24 22:49 ` [RFC][PATCH 2/2] Fix evsel.c misaligned address errors Numfor Mbiziwo-Tiapo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).