[PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Namhyung Kim]

The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
attribute and IDs.  The ID table comes after the attr and it calculate
size of the table using the total record size and the attr size.

  n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)

This is fine for most use cases, but sometimes it saves the pipe output
in a file and then process it later.  And it becomes a problem if there
is a change in attr size between the record and report.

  $ perf record -o- > perf-pipe.data  # old version
  $ perf report -i- < perf-pipe.data  # new version

For example, if the attr size is 128 and it has 4 IDs, then it would
save them in 168 byte like below:

   8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
 128 byte: perf event attr { .size = 128, ... },
  32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },

But when report later, it thinks the attr size is 136 then it only read
the last 3 entries as ID.

   8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
 136 byte: perf event attr { .size = 136, ... },
  24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing

So it should use the recorded version of the attr.  The attr has the
size field already then it should honor the size when reading data.

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

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 52fbf526fe74..f89321cbfdee 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
 			     union perf_event *event,
 			     struct evlist **pevlist)
 {
-	u32 i, ids, n_ids;
+	u32 i, n_ids;
+	u64 *ids;
 	struct evsel *evsel;
 	struct evlist *evlist = *pevlist;
 
@@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
 
 	evlist__add(evlist, evsel);
 
-	ids = event->header.size;
-	ids -= (void *)&event->attr.id - (void *)event;
-	n_ids = ids / sizeof(u64);
+	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
+	n_ids = n_ids / sizeof(u64);
 	/*
 	 * We don't have the cpu and thread maps on the header, so
 	 * for allocating the perf_sample_id table we fake 1 cpu and
@@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
 	if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
 		return -ENOMEM;
 
+	ids = (void *)&event->attr.attr + event->attr.attr.size;
 	for (i = 0; i < n_ids; i++) {
-		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
+		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
 	}
 
 	return 0;
-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Ian Rogers]

On Sun, Aug 6, 2023 at 11:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
> attribute and IDs.  The ID table comes after the attr and it calculate
> size of the table using the total record size and the attr size.
>
>   n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)
>
> This is fine for most use cases, but sometimes it saves the pipe output
> in a file and then process it later.  And it becomes a problem if there
> is a change in attr size between the record and report.
>
>   $ perf record -o- > perf-pipe.data  # old version
>   $ perf report -i- < perf-pipe.data  # new version
>
> For example, if the attr size is 128 and it has 4 IDs, then it would
> save them in 168 byte like below:
>
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  128 byte: perf event attr { .size = 128, ... },
>   32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },
>
> But when report later, it thinks the attr size is 136 then it only read
> the last 3 entries as ID.
>
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  136 byte: perf event attr { .size = 136, ... },
>   24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing
>
> So it should use the recorded version of the attr.  The attr has the
> size field already then it should honor the size when reading data.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

Thanks,
Ian

> ---
>  tools/perf/util/header.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 52fbf526fe74..f89321cbfdee 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>                              union perf_event *event,
>                              struct evlist **pevlist)
>  {
> -       u32 i, ids, n_ids;
> +       u32 i, n_ids;
> +       u64 *ids;
>         struct evsel *evsel;
>         struct evlist *evlist = *pevlist;
>
> @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>
>         evlist__add(evlist, evsel);
>
> -       ids = event->header.size;
> -       ids -= (void *)&event->attr.id - (void *)event;
> -       n_ids = ids / sizeof(u64);
> +       n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +       n_ids = n_ids / sizeof(u64);
>         /*
>          * We don't have the cpu and thread maps on the header, so
>          * for allocating the perf_sample_id table we fake 1 cpu and
> @@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>         if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
>                 return -ENOMEM;
>
> +       ids = (void *)&event->attr.attr + event->attr.attr.size;
>         for (i = 0; i < n_ids; i++) {
> -               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
> +               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
>         }
>
>         return 0;
> --
> 2.41.0.640.ga95def55d0-goog
>

Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Adrian Hunter]

On 7/08/23 09:16, Namhyung Kim wrote:
> The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
> attribute and IDs.  The ID table comes after the attr and it calculate
> size of the table using the total record size and the attr size.
> 
>   n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)
> 
> This is fine for most use cases, but sometimes it saves the pipe output
> in a file and then process it later.  And it becomes a problem if there
> is a change in attr size between the record and report.
> 
>   $ perf record -o- > perf-pipe.data  # old version
>   $ perf report -i- < perf-pipe.data  # new version
> 
> For example, if the attr size is 128 and it has 4 IDs, then it would
> save them in 168 byte like below:
> 
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  128 byte: perf event attr { .size = 128, ... },
>   32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },
> 
> But when report later, it thinks the attr size is 136 then it only read
> the last 3 entries as ID.
> 
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  136 byte: perf event attr { .size = 136, ... },
>   24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing
> 
> So it should use the recorded version of the attr.  The attr has the
> size field already then it should honor the size when reading data.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/header.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 52fbf526fe74..f89321cbfdee 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>  			     union perf_event *event,
>  			     struct evlist **pevlist)
>  {
> -	u32 i, ids, n_ids;
> +	u32 i, n_ids;
> +	u64 *ids;
>  	struct evsel *evsel;
>  	struct evlist *evlist = *pevlist;
>  
> @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>  
>  	evlist__add(evlist, evsel);
>  
> -	ids = event->header.size;
> -	ids -= (void *)&event->attr.id - (void *)event;
> -	n_ids = ids / sizeof(u64);
> +	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids = n_ids / sizeof(u64);
>  	/*
>  	 * We don't have the cpu and thread maps on the header, so
>  	 * for allocating the perf_sample_id table we fake 1 cpu and
> @@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>  	if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
>  		return -ENOMEM;
>  
> +	ids = (void *)&event->attr.attr + event->attr.attr.size;
>  	for (i = 0; i < n_ids; i++) {
> -		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
> +		perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
>  	}
>  
>  	return 0;

This is a good catch!

It looks like perf_event__hdr_swap() might also have this problem.

I wonder if we should remove 'id' from struct perf_record_header_attr
since the position is not guaranteed?

Probably could use a comment there either way.

Also perhaps a fixes tag and cc stable

Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Namhyung Kim]

HI Adrian,

Sorry for the late reply.

On Mon, Aug 14, 2023 at 12:11 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 7/08/23 09:16, Namhyung Kim wrote:
> > The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
> > attribute and IDs.  The ID table comes after the attr and it calculate
> > size of the table using the total record size and the attr size.
> >
> >   n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)
> >
> > This is fine for most use cases, but sometimes it saves the pipe output
> > in a file and then process it later.  And it becomes a problem if there
> > is a change in attr size between the record and report.
> >
> >   $ perf record -o- > perf-pipe.data  # old version
> >   $ perf report -i- < perf-pipe.data  # new version
> >
> > For example, if the attr size is 128 and it has 4 IDs, then it would
> > save them in 168 byte like below:
> >
> >    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
> >  128 byte: perf event attr { .size = 128, ... },
> >   32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },
> >
> > But when report later, it thinks the attr size is 136 then it only read
> > the last 3 entries as ID.
> >
> >    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
> >  136 byte: perf event attr { .size = 136, ... },
> >   24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing
> >
> > So it should use the recorded version of the attr.  The attr has the
> > size field already then it should honor the size when reading data.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/header.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 52fbf526fe74..f89321cbfdee 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >                            union perf_event *event,
> >                            struct evlist **pevlist)
> >  {
> > -     u32 i, ids, n_ids;
> > +     u32 i, n_ids;
> > +     u64 *ids;
> >       struct evsel *evsel;
> >       struct evlist *evlist = *pevlist;
> >
> > @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >
> >       evlist__add(evlist, evsel);
> >
> > -     ids = event->header.size;
> > -     ids -= (void *)&event->attr.id - (void *)event;
> > -     n_ids = ids / sizeof(u64);
> > +     n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> > +     n_ids = n_ids / sizeof(u64);
> >       /*
> >        * We don't have the cpu and thread maps on the header, so
> >        * for allocating the perf_sample_id table we fake 1 cpu and
> > @@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
> >       if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
> >               return -ENOMEM;
> >
> > +     ids = (void *)&event->attr.attr + event->attr.attr.size;
> >       for (i = 0; i < n_ids; i++) {
> > -             perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
> > +             perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
> >       }
> >
> >       return 0;
>
> This is a good catch!
>
> It looks like perf_event__hdr_swap() might also have this problem.

You mean perf_event__hdr_attr_swap(), right?  Yeah, looks so.
I'll change it too in v2.

>
> I wonder if we should remove 'id' from struct perf_record_header_attr
> since the position is not guaranteed?
>
> Probably could use a comment there either way.

Sounds good.  I'll remove the id field and add a comment.

>
> Also perhaps a fixes tag and cc stable

Sure, thanks for the review!

Namhyung