[PATCH] perf tools: Avoid unaligned pointer operations

[PATCH] perf tools: Avoid unaligned pointer operations

[Namhyung Kim]

The sample data is 64-bit aligned basically but raw data starts with
32-bit length field and data follows.  In perf_event__synthesize_sample
it treats the sample data as a 64-bit array.  And it needs some trick
to update the raw data properly.

But it seems some compilers are not happy with this and the program dies
siliently.  I found the sample parsing test failed without any messages
on affected systems.

Let's update the code to use a 32-bit pointer directly and make sure the
result is 64-bit aligned again.  No functional changes intended.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/synthetic-events.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index a58444c4aed1f1ea..385383ef6cf1edaf 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
 	}
 
 	if (type & PERF_SAMPLE_RAW) {
-		u.val32[0] = sample->raw_size;
-		*array = u.val64;
-		array = (void *)array + sizeof(u32);
+		u32 *array32 = (void *)array;
+
+		*array32 = sample->raw_size;
+		array32++;
+
+		memcpy(array32, sample->raw_data, sample->raw_size);
+		array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
 
-		memcpy(array, sample->raw_data, sample->raw_size);
-		array = (void *)array + sample->raw_size;
+		/* make sure the array is 64-bit aligned */
+		BUG_ON(((long)array) / sizeof(u64));
 	}
 
 	if (type & PERF_SAMPLE_BRANCH_STACK) {
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Ian Rogers]

On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The sample data is 64-bit aligned basically but raw data starts with
> 32-bit length field and data follows.  In perf_event__synthesize_sample
> it treats the sample data as a 64-bit array.  And it needs some trick
> to update the raw data properly.
>
> But it seems some compilers are not happy with this and the program dies
> siliently.  I found the sample parsing test failed without any messages
> on affected systems.
>
> Let's update the code to use a 32-bit pointer directly and make sure the
> result is 64-bit aligned again.  No functional changes intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/synthetic-events.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
>         }
>
>         if (type & PERF_SAMPLE_RAW) {
> -               u.val32[0] = sample->raw_size;
> -               *array = u.val64;
> -               array = (void *)array + sizeof(u32);
> +               u32 *array32 = (void *)array;
> +
> +               *array32 = sample->raw_size;
> +               array32++;
> +
> +               memcpy(array32, sample->raw_data, sample->raw_size);
> +               array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
>
> -               memcpy(array, sample->raw_data, sample->raw_size);
> -               array = (void *)array + sample->raw_size;
> +               /* make sure the array is 64-bit aligned */
> +               BUG_ON(((long)array) / sizeof(u64));
>         }
>
>         if (type & PERF_SAMPLE_BRANCH_STACK) {
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Arnaldo Carvalho de Melo]

On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote:
> On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > The sample data is 64-bit aligned basically but raw data starts with
> > 32-bit length field and data follows.  In perf_event__synthesize_sample
> > it treats the sample data as a 64-bit array.  And it needs some trick
> > to update the raw data properly.

> > But it seems some compilers are not happy with this and the program dies
> > siliently.  I found the sample parsing test failed without any messages
> > on affected systems.

> > Let's update the code to use a 32-bit pointer directly and make sure the
> > result is 64-bit aligned again.  No functional changes intended.

> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
 
> Reviewed-by: Ian Rogers <irogers@google.com>

Looks good, applied to perf-tools-next since this is something that is
not new nor looks urgent.

I think that since we have multiple maintainers, one for not urgent
stuff/development and the other for the current window/urgent stuff,
that we should express the expectation about where a patch should be
processed, by having on the subject the tree the submitter thinks should
take the patch, i.e. for this one:

[PATCH next] perf tools: Avoid unaligned pointer operations

While for urgent stuff we could do:

[PATCH urgent] perf tools: Avoid unaligned pointer operations

wdyt?

- Arnaldo

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Namhyung Kim]

On Thu, Dec 12, 2024 at 03:41:02PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote:
> > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > The sample data is 64-bit aligned basically but raw data starts with
> > > 32-bit length field and data follows.  In perf_event__synthesize_sample
> > > it treats the sample data as a 64-bit array.  And it needs some trick
> > > to update the raw data properly.
> 
> > > But it seems some compilers are not happy with this and the program dies
> > > siliently.  I found the sample parsing test failed without any messages
> > > on affected systems.
> 
> > > Let's update the code to use a 32-bit pointer directly and make sure the
> > > result is 64-bit aligned again.  No functional changes intended.
> 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>  
> > Reviewed-by: Ian Rogers <irogers@google.com>
> 
> Looks good, applied to perf-tools-next since this is something that is
> not new nor looks urgent.
> 
> I think that since we have multiple maintainers, one for not urgent
> stuff/development and the other for the current window/urgent stuff,
> that we should express the expectation about where a patch should be
> processed, by having on the subject the tree the submitter thinks should
> take the patch, i.e. for this one:
> 
> [PATCH next] perf tools: Avoid unaligned pointer operations
> 
> While for urgent stuff we could do:
> 
> [PATCH urgent] perf tools: Avoid unaligned pointer operations
> 
> wdyt?

Looks good.  It'd be really great if contributors can do this.

But I also think 'next' should be the default so only 'urgent' would be
specified if needed.

Thanks,
Namhyung

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Ian Rogers]

On Thu, Dec 12, 2024 at 1:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Dec 12, 2024 at 03:41:02PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote:
> > > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > The sample data is 64-bit aligned basically but raw data starts with
> > > > 32-bit length field and data follows.  In perf_event__synthesize_sample
> > > > it treats the sample data as a 64-bit array.  And it needs some trick
> > > > to update the raw data properly.
> >
> > > > But it seems some compilers are not happy with this and the program dies
> > > > siliently.  I found the sample parsing test failed without any messages
> > > > on affected systems.
> >
> > > > Let's update the code to use a 32-bit pointer directly and make sure the
> > > > result is 64-bit aligned again.  No functional changes intended.
> >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > Looks good, applied to perf-tools-next since this is something that is
> > not new nor looks urgent.
> >
> > I think that since we have multiple maintainers, one for not urgent
> > stuff/development and the other for the current window/urgent stuff,
> > that we should express the expectation about where a patch should be
> > processed, by having on the subject the tree the submitter thinks should
> > take the patch, i.e. for this one:
> >
> > [PATCH next] perf tools: Avoid unaligned pointer operations
> >
> > While for urgent stuff we could do:
> >
> > [PATCH urgent] perf tools: Avoid unaligned pointer operations
> >
> > wdyt?
>
> Looks good.  It'd be really great if contributors can do this.
>
> But I also think 'next' should be the default so only 'urgent' would be
> specified if needed.

Fwiw, I needed this fix, forgot about this change, and wrote my own by
just sinking the unaligned array computation (that causes undefined
behavior) into where it was used:
```
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1690,10 +1690,9 @@ int perf_event__synthesize_sample(union
perf_event *event, u64 type, u64 read_fo
        if (type & PERF_SAMPLE_RAW) {
                u.val32[0] = sample->raw_size;
                *array = u.val64;
-               array = (void *)array + sizeof(u32);

-               memcpy(array, sample->raw_data, sample->raw_size);
-               array = (void *)array + sample->raw_size;
+               memcpy((void *)array + sizeof(u32), sample->raw_data,
sample->raw_size);
+               array = (void *)array + sizeof(u32) + sample->raw_size;
        }

        if (type & PERF_SAMPLE_BRANCH_STACK) {
````
Namhyung's change is better because of the BUG_ON. Perhaps that BUG_ON
should appear after all the void* math that can create unaligned u64
pointers in this function.

Thanks,
Ian

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Ian Rogers]

On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The sample data is 64-bit aligned basically but raw data starts with
> 32-bit length field and data follows.  In perf_event__synthesize_sample
> it treats the sample data as a 64-bit array.  And it needs some trick
> to update the raw data properly.
>
> But it seems some compilers are not happy with this and the program dies
> siliently.  I found the sample parsing test failed without any messages
> on affected systems.
>
> Let's update the code to use a 32-bit pointer directly and make sure the
> result is 64-bit aligned again.  No functional changes intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/synthetic-events.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
>         }
>
>         if (type & PERF_SAMPLE_RAW) {
> -               u.val32[0] = sample->raw_size;
> -               *array = u.val64;
> -               array = (void *)array + sizeof(u32);
> +               u32 *array32 = (void *)array;
> +
> +               *array32 = sample->raw_size;
> +               array32++;
> +
> +               memcpy(array32, sample->raw_data, sample->raw_size);
> +               array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
>
> -               memcpy(array, sample->raw_data, sample->raw_size);
> -               array = (void *)array + sample->raw_size;
> +               /* make sure the array is 64-bit aligned */
> +               BUG_ON(((long)array) / sizeof(u64));

I think you intended:

BUG_ON(((long)array) % sizeof(u64));

Thanks,
Ian

>         }
>
>         if (type & PERF_SAMPLE_BRANCH_STACK) {
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH] perf tools: Avoid unaligned pointer operations

[Namhyung Kim]

On Thu, Dec 12, 2024 at 03:56:31PM -0800, Ian Rogers wrote:
> On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The sample data is 64-bit aligned basically but raw data starts with
> > 32-bit length field and data follows.  In perf_event__synthesize_sample
> > it treats the sample data as a 64-bit array.  And it needs some trick
> > to update the raw data properly.
> >
> > But it seems some compilers are not happy with this and the program dies
> > siliently.  I found the sample parsing test failed without any messages
> > on affected systems.
> >
> > Let's update the code to use a 32-bit pointer directly and make sure the
> > result is 64-bit aligned again.  No functional changes intended.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/synthetic-events.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
> >         }
> >
> >         if (type & PERF_SAMPLE_RAW) {
> > -               u.val32[0] = sample->raw_size;
> > -               *array = u.val64;
> > -               array = (void *)array + sizeof(u32);
> > +               u32 *array32 = (void *)array;
> > +
> > +               *array32 = sample->raw_size;
> > +               array32++;
> > +
> > +               memcpy(array32, sample->raw_data, sample->raw_size);
> > +               array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
> >
> > -               memcpy(array, sample->raw_data, sample->raw_size);
> > -               array = (void *)array + sample->raw_size;
> > +               /* make sure the array is 64-bit aligned */
> > +               BUG_ON(((long)array) / sizeof(u64));
> 
> I think you intended:
> 
> BUG_ON(((long)array) % sizeof(u64));

Yep, fixed in v2.

https://lore.kernel.org/r/20241128010325.946897-1-namhyung@kernel.org

Thanks,
Namhyung