[PATCH 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[PATCH 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Petr Malat]

Both mmapped and compressed events can be split by the buffer boundary,
it doesn't make sense to handle them differently.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 44 +++++++++++++++------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7a5f03764702..593d5a823eba 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2024,8 +2024,8 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
 }
 
 static union perf_event *
-prefetch_event(char *buf, u64 head, size_t mmap_size,
-	       bool needs_swap, union perf_event *error)
+fetch_mmaped_event(struct perf_session *session,
+		   u64 head, size_t mmap_size, char *buf)
 {
 	union perf_event *event;
 
@@ -2037,32 +2037,20 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
 		return NULL;
 
 	event = (union perf_event *)(buf + head);
-	if (needs_swap)
-		perf_event_header__bswap(&event->header);
-
-	if (head + event->header.size <= mmap_size)
-		return event;
 
-	/* We're not fetching the event so swap back again */
-	if (needs_swap)
+	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
-		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
-
-	return error;
-}
-
-static union perf_event *
-fetch_mmaped_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
-{
-	return prefetch_event(buf, head, mmap_size, needs_swap, ERR_PTR(-EINVAL));
-}
+	if (head + event->header.size > mmap_size) {
+		/* We're not fetching the event so swap back again */
+		if (session->header.needs_swap)
+			perf_event_header__bswap(&event->header);
+		pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n",
+			 __func__, head, event->header.size, mmap_size);
+		return ERR_PTR(-EINVAL);
+	}
 
-static union perf_event *
-fetch_decomp_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
-{
-	return prefetch_event(buf, head, mmap_size, needs_swap, NULL);
+	return event;
 }
 
 static int __perf_session__process_decomp_events(struct perf_session *session)
@@ -2075,8 +2063,10 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
 		return 0;
 
 	while (decomp->head < decomp->size && !session_done()) {
-		union perf_event *event = fetch_decomp_event(decomp->head, decomp->size, decomp->data,
-							     session->header.needs_swap);
+		union perf_event *event = fetch_mmaped_event(session, decomp->head, decomp->size, decomp->data);
+
+		if (IS_ERR(event))
+			return PTR_ERR(event);
 
 		if (!event)
 			break;
@@ -2176,7 +2166,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	}
 
 more:
-	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
+	event = fetch_mmaped_event(session, head, mmap_size, buf);
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
-- 
2.20.1

[PATCH 2/3] Revert "perf session: Avoid infinite loop when seeing invalid header.size"

[Petr Malat]

An event may be split by a buffer boundary, we should not abort processing
if that happens.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 593d5a823eba..e57b0d09d196 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
 #include <inttypes.h>
-#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/zalloc.h>
 #include <api/fs/fs.h>
@@ -2045,9 +2044,7 @@ fetch_mmaped_event(struct perf_session *session,
 		/* We're not fetching the event so swap back again */
 		if (session->header.needs_swap)
 			perf_event_header__bswap(&event->header);
-		pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n",
-			 __func__, head, event->header.size, mmap_size);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 
 	return event;
@@ -2065,9 +2062,6 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
 	while (decomp->head < decomp->size && !session_done()) {
 		union perf_event *event = fetch_mmaped_event(session, decomp->head, decomp->size, decomp->data);
 
-		if (IS_ERR(event))
-			return PTR_ERR(event);
-
 		if (!event)
 			break;
 
@@ -2167,9 +2161,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 more:
 	event = fetch_mmaped_event(session, head, mmap_size, buf);
-	if (IS_ERR(event))
-		return PTR_ERR(event);
-
 	if (!event) {
 		if (mmaps[map_idx]) {
 			munmap(mmaps[map_idx], mmap_size);
-- 
2.20.1

[PATCH 3/3] perf session: Avoid infinite loop if an event is truncated

[Petr Malat]

If an event was truncated at the end of a perf.data file larger than
MAP_SIZE, the event reading code ended up in an infinite loop. Break
this loop by making sure the mapping window is always shifting
towards the end of the file.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index e57b0d09d196..b73b85d99628 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2168,6 +2168,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		}
 
 		page_offset = page_size * (head / page_size);
+		if (!page_offset) {
+			pr_err("%#" PRIx64 " [%#x]: truncated event\n",
+					file_offset + head);
+			err = -EINVAL;
+			goto out;
+		}
 		file_offset += page_offset;
 		head -= page_offset;
 		goto remap;
-- 
2.20.1

Re: [PATCH 3/3] perf session: Avoid infinite loop if an event is truncated

[Petr Malat]

I made a rebase mistake and picked old change, I will send the series
again rebased on v5.10-rc5. Sorry for the inconvenience.
  Petr

On Tue, Nov 24, 2020 at 10:59:21AM +0100, Petr Malat wrote:
> If an event was truncated at the end of a perf.data file larger than
> MAP_SIZE, the event reading code ended up in an infinite loop. Break
> this loop by making sure the mapping window is always shifting
> towards the end of the file.
> 
> Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
> Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  tools/perf/util/session.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index e57b0d09d196..b73b85d99628 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2168,6 +2168,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  		}
>  
>  		page_offset = page_size * (head / page_size);
> +		if (!page_offset) {
> +			pr_err("%#" PRIx64 " [%#x]: truncated event\n",
> +					file_offset + head);
> +			err = -EINVAL;
> +			goto out;
> +		}
>  		file_offset += page_offset;
>  		head -= page_offset;
>  		goto remap;
> -- 
> 2.20.1
> 

[PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Petr Malat]

Both mmapped and compressed events can be split by the buffer boundary,
it doesn't make sense to handle them differently.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 44 +++++++++++++++------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 098080287c68..0d7a59c1aeb6 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2038,8 +2038,8 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
 }
 
 static union perf_event *
-prefetch_event(char *buf, u64 head, size_t mmap_size,
-	       bool needs_swap, union perf_event *error)
+fetch_mmaped_event(struct perf_session *session,
+		   u64 head, size_t mmap_size, char *buf)
 {
 	union perf_event *event;
 
@@ -2051,32 +2051,20 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
 		return NULL;
 
 	event = (union perf_event *)(buf + head);
-	if (needs_swap)
-		perf_event_header__bswap(&event->header);
-
-	if (head + event->header.size <= mmap_size)
-		return event;
 
-	/* We're not fetching the event so swap back again */
-	if (needs_swap)
+	if (session->header.needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
-		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
-
-	return error;
-}
-
-static union perf_event *
-fetch_mmaped_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
-{
-	return prefetch_event(buf, head, mmap_size, needs_swap, ERR_PTR(-EINVAL));
-}
+	if (head + event->header.size > mmap_size) {
+		/* We're not fetching the event so swap back again */
+		if (session->header.needs_swap)
+			perf_event_header__bswap(&event->header);
+		pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n",
+			 __func__, head, event->header.size, mmap_size);
+		return ERR_PTR(-EINVAL);
+	}
 
-static union perf_event *
-fetch_decomp_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
-{
-	return prefetch_event(buf, head, mmap_size, needs_swap, NULL);
+	return event;
 }
 
 static int __perf_session__process_decomp_events(struct perf_session *session)
@@ -2089,8 +2077,10 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
 		return 0;
 
 	while (decomp->head < decomp->size && !session_done()) {
-		union perf_event *event = fetch_decomp_event(decomp->head, decomp->size, decomp->data,
-							     session->header.needs_swap);
+		union perf_event *event = fetch_mmaped_event(session, decomp->head, decomp->size, decomp->data);
+
+		if (IS_ERR(event))
+			return PTR_ERR(event);
 
 		if (!event)
 			break;
@@ -2190,7 +2180,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 	}
 
 more:
-	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
+	event = fetch_mmaped_event(session, head, mmap_size, buf);
 	if (IS_ERR(event))
 		return PTR_ERR(event);
 
-- 
2.20.1

[PATCH v2 2/3] Revert "perf session: Avoid infinite loop when seeing invalid header.size"

[Petr Malat]

An event may be split by a buffer boundary, we should not abort processing
if that happens.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 0d7a59c1aeb6..5f7bc5ad620f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
 #include <inttypes.h>
-#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/zalloc.h>
 #include <api/fs/fs.h>
@@ -2059,9 +2058,7 @@ fetch_mmaped_event(struct perf_session *session,
 		/* We're not fetching the event so swap back again */
 		if (session->header.needs_swap)
 			perf_event_header__bswap(&event->header);
-		pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n",
-			 __func__, head, event->header.size, mmap_size);
-		return ERR_PTR(-EINVAL);
+		return NULL;
 	}
 
 	return event;
@@ -2079,9 +2076,6 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
 	while (decomp->head < decomp->size && !session_done()) {
 		union perf_event *event = fetch_mmaped_event(session, decomp->head, decomp->size, decomp->data);
 
-		if (IS_ERR(event))
-			return PTR_ERR(event);
-
 		if (!event)
 			break;
 
@@ -2181,9 +2175,6 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 more:
 	event = fetch_mmaped_event(session, head, mmap_size, buf);
-	if (IS_ERR(event))
-		return PTR_ERR(event);
-
 	if (!event) {
 		if (mmaps[map_idx]) {
 			munmap(mmaps[map_idx], mmap_size);
-- 
2.20.1

[PATCH v2 3/3] perf session: Avoid infinite loop if an event is truncated

[Petr Malat]

If an event was truncated at the end of a perf.data file larger than
MAP_SIZE, the event reading code ended up in an infinite loop. Break
this loop by making sure the mapping window is always shifting
towards the end of the file.

Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Petr Malat <oss@malat.biz>
---
 tools/perf/util/session.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 5f7bc5ad620f..79b2e93c1639 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
 #include <inttypes.h>
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/zalloc.h>
 #include <api/fs/fs.h>
@@ -2182,6 +2183,12 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 		}
 
 		page_offset = page_size * (head / page_size);
+		if (!page_offset) {
+			pr_err("%#" PRIx64 ": truncated event\n",
+					file_offset + head);
+			err = -EINVAL;
+			goto out;
+		}
 		file_offset += page_offset;
 		head -= page_offset;
 		goto remap;
-- 
2.20.1

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Jiri Olsa]

On Tue, Nov 24, 2020 at 11:29:15AM +0100, Petr Malat wrote:
> Both mmapped and compressed events can be split by the buffer boundary,
> it doesn't make sense to handle them differently.

hi,
I'm going to need more than this, if there's a problem
with current code please share more details, what's
broken and how it shows

thanks,
jirka

> 
> Fixes: bb1835a3b86c ("perf session: Fix decompression of PERF_RECORD_COMPRESSED records")
> Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  tools/perf/util/session.c | 44 +++++++++++++++------------------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 098080287c68..0d7a59c1aeb6 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2038,8 +2038,8 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
>  }
>  
>  static union perf_event *
> -prefetch_event(char *buf, u64 head, size_t mmap_size,
> -	       bool needs_swap, union perf_event *error)
> +fetch_mmaped_event(struct perf_session *session,
> +		   u64 head, size_t mmap_size, char *buf)
>  {
>  	union perf_event *event;
>  
> @@ -2051,32 +2051,20 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
>  		return NULL;
>  
>  	event = (union perf_event *)(buf + head);
> -	if (needs_swap)
> -		perf_event_header__bswap(&event->header);
> -
> -	if (head + event->header.size <= mmap_size)
> -		return event;
>  
> -	/* We're not fetching the event so swap back again */
> -	if (needs_swap)
> +	if (session->header.needs_swap)
>  		perf_event_header__bswap(&event->header);
>  
> -	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
> -		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
> -
> -	return error;
> -}
> -
> -static union perf_event *
> -fetch_mmaped_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
> -{
> -	return prefetch_event(buf, head, mmap_size, needs_swap, ERR_PTR(-EINVAL));
> -}
> +	if (head + event->header.size > mmap_size) {
> +		/* We're not fetching the event so swap back again */
> +		if (session->header.needs_swap)
> +			perf_event_header__bswap(&event->header);
> +		pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n",
> +			 __func__, head, event->header.size, mmap_size);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
> -static union perf_event *
> -fetch_decomp_event(u64 head, size_t mmap_size, char *buf, bool needs_swap)
> -{
> -	return prefetch_event(buf, head, mmap_size, needs_swap, NULL);
> +	return event;
>  }
>  
>  static int __perf_session__process_decomp_events(struct perf_session *session)
> @@ -2089,8 +2077,10 @@ static int __perf_session__process_decomp_events(struct perf_session *session)
>  		return 0;
>  
>  	while (decomp->head < decomp->size && !session_done()) {
> -		union perf_event *event = fetch_decomp_event(decomp->head, decomp->size, decomp->data,
> -							     session->header.needs_swap);
> +		union perf_event *event = fetch_mmaped_event(session, decomp->head, decomp->size, decomp->data);
> +
> +		if (IS_ERR(event))
> +			return PTR_ERR(event);
>  
>  		if (!event)
>  			break;
> @@ -2190,7 +2180,7 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  	}
>  
>  more:
> -	event = fetch_mmaped_event(head, mmap_size, buf, session->header.needs_swap);
> +	event = fetch_mmaped_event(session, head, mmap_size, buf);
>  	if (IS_ERR(event))
>  		return PTR_ERR(event);
>  
> -- 
> 2.20.1
> 

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Petr Malat]

Hi!
On Tue, Nov 24, 2020 at 03:36:45PM +0100, Jiri Olsa wrote:
> On Tue, Nov 24, 2020 at 11:29:15AM +0100, Petr Malat wrote:
> > Both mmapped and compressed events can be split by the buffer boundary,
> > it doesn't make sense to handle them differently.
> I'm going to need more than this, if there's a problem
> with current code please share more details, what's
> broken and how it shows
It's easy to trigger the problem - make a perf recording larger than
MMAP_SIZE (32MB on 32-bit platform) and run perf report on it. There
is a small chance recorded events will be aligned on the 32 MB
boundary and in that case just repeat the test.

The problem was introduced by "perf session: Avoid infinite loop when
seeing invalid header.size", which instead of aborting the execution
when there is a truncated event at the end of the file just terminated
execution whenever there is a split event. Later then the problem has
been noticed for compressed events and fixed by "perf session: Fix
decompression of PERF_RECORD_COMPRESSED records" by effectively
reverting "perf session: Avoid infinite loop when seeing invalid
header.size" for compressed events, which left uncompressed events
broken.

I think the best is to revert these 2 changes and fix the original
problem by aborting when there is no actual shift during remapping - as
long as we shift, it's clear we must approach the end of the file so
such an algorithm can't loop forever.
BR,
  Petr

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Petr Malat]

Hi Jiří,
were you able to reproduce the issue? I may also upload perf-archive
if that would help.
  Petr

On Tue, Nov 24, 2020 at 07:15:19PM +0100, Petr Malat wrote:
> Hi!
> On Tue, Nov 24, 2020 at 03:36:45PM +0100, Jiri Olsa wrote:
> > On Tue, Nov 24, 2020 at 11:29:15AM +0100, Petr Malat wrote:
> > > Both mmapped and compressed events can be split by the buffer boundary,
> > > it doesn't make sense to handle them differently.
> > I'm going to need more than this, if there's a problem
> > with current code please share more details, what's
> > broken and how it shows
> It's easy to trigger the problem - make a perf recording larger than
> MMAP_SIZE (32MB on 32-bit platform) and run perf report on it. There
> is a small chance recorded events will be aligned on the 32 MB
> boundary and in that case just repeat the test.
> 
> The problem was introduced by "perf session: Avoid infinite loop when
> seeing invalid header.size", which instead of aborting the execution
> when there is a truncated event at the end of the file just terminated
> execution whenever there is a split event. Later then the problem has
> been noticed for compressed events and fixed by "perf session: Fix
> decompression of PERF_RECORD_COMPRESSED records" by effectively
> reverting "perf session: Avoid infinite loop when seeing invalid
> header.size" for compressed events, which left uncompressed events
> broken.
> 
> I think the best is to revert these 2 changes and fix the original
> problem by aborting when there is no actual shift during remapping - as
> long as we shift, it's clear we must approach the end of the file so
> such an algorithm can't loop forever.
> BR,
>   Petr

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Jiri Olsa]

On Mon, Nov 30, 2020 at 12:40:20PM +0100, Petr Malat wrote:
> Hi Jiří,
> were you able to reproduce the issue? I may also upload perf-archive
> if that would help.

oh yea ;-) seems like those 2 commits you reverted broke 32 bits
perf for data files > 32MB

but the fix you did does not work for Alexey's test he mentioned
in the commit:

      $ perf record -z -- some_long_running_workload
      $ perf report --stdio -vv

it's failing for me with:

	# ./perf report
	Couldn't allocate memory for decompression
	0xfe6f3a [0x60]: failed to process type: 81 [Operation not permitted]
	Error:
	failed to process sample
	# To display the perf.data header info, please use --header/--header-only options.
	#

I think that's why here's special handling for compressed
events, but I'll need to check on that in more detail,
I was hoping for Alexey to answer ;-)

jirka

>   Petr
> 
> On Tue, Nov 24, 2020 at 07:15:19PM +0100, Petr Malat wrote:
> > Hi!
> > On Tue, Nov 24, 2020 at 03:36:45PM +0100, Jiri Olsa wrote:
> > > On Tue, Nov 24, 2020 at 11:29:15AM +0100, Petr Malat wrote:
> > > > Both mmapped and compressed events can be split by the buffer boundary,
> > > > it doesn't make sense to handle them differently.
> > > I'm going to need more than this, if there's a problem
> > > with current code please share more details, what's
> > > broken and how it shows
> > It's easy to trigger the problem - make a perf recording larger than
> > MMAP_SIZE (32MB on 32-bit platform) and run perf report on it. There
> > is a small chance recorded events will be aligned on the 32 MB
> > boundary and in that case just repeat the test.
> > 
> > The problem was introduced by "perf session: Avoid infinite loop when
> > seeing invalid header.size", which instead of aborting the execution
> > when there is a truncated event at the end of the file just terminated
> > execution whenever there is a split event. Later then the problem has
> > been noticed for compressed events and fixed by "perf session: Fix
> > decompression of PERF_RECORD_COMPRESSED records" by effectively
> > reverting "perf session: Avoid infinite loop when seeing invalid
> > header.size" for compressed events, which left uncompressed events
> > broken.
> > 
> > I think the best is to revert these 2 changes and fix the original
> > problem by aborting when there is no actual shift during remapping - as
> > long as we shift, it's clear we must approach the end of the file so
> > such an algorithm can't loop forever.
> > BR,
> >   Petr
> 

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Alexei Budankov]

On 01.12.2020 22:09, Jiri Olsa wrote:
> On Mon, Nov 30, 2020 at 12:40:20PM +0100, Petr Malat wrote:
>> Hi Jiří,
>> were you able to reproduce the issue? I may also upload perf-archive
>> if that would help.
> 
> oh yea ;-) seems like those 2 commits you reverted broke 32 bits
> perf for data files > 32MB
> 
> but the fix you did does not work for Alexey's test he mentioned
> in the commit:
> 
>       $ perf record -z -- some_long_running_workload
>       $ perf report --stdio -vv
> 
> it's failing for me with:
> 
> 	# ./perf report
> 	Couldn't allocate memory for decompression
> 	0xfe6f3a [0x60]: failed to process type: 81 [Operation not permitted]
> 	Error:
> 	failed to process sample
> 	# To display the perf.data header info, please use --header/--header-only options.
> 	#
> 
> I think that's why here's special handling for compressed
> events, but I'll need to check on that in more detail,
> I was hoping for Alexey to answer ;-)

Sorry for delay. Alexey Bayduraev could possibly help with that sooner.
Alexey, could you please follow up.

Thanks,
Alexei

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Alexei Budankov]

Eventually sending to the proper Alexey's address.

On 02.12.2020 0:04, Alexei Budankov wrote:
> 
> On 01.12.2020 22:09, Jiri Olsa wrote:
>> On Mon, Nov 30, 2020 at 12:40:20PM +0100, Petr Malat wrote:
>>> Hi Jiří,
>>> were you able to reproduce the issue? I may also upload perf-archive
>>> if that would help.
>>
>> oh yea ;-) seems like those 2 commits you reverted broke 32 bits
>> perf for data files > 32MB
>>
>> but the fix you did does not work for Alexey's test he mentioned
>> in the commit:
>>
>>       $ perf record -z -- some_long_running_workload
>>       $ perf report --stdio -vv
>>
>> it's failing for me with:
>>
>> 	# ./perf report
>> 	Couldn't allocate memory for decompression
>> 	0xfe6f3a [0x60]: failed to process type: 81 [Operation not permitted]
>> 	Error:
>> 	failed to process sample
>> 	# To display the perf.data header info, please use --header/--header-only options.
>> 	#
>>
>> I think that's why here's special handling for compressed
>> events, but I'll need to check on that in more detail,
>> I was hoping for Alexey to answer ;-)
> 
> Sorry for delay. Alexey Bayduraev could possibly help with that sooner.
> Alexey, could you please follow up.
> 
> Thanks,
> Alexei

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Bayduraev, Alexey V]

Hi,

I was able to reproduce "Couldn't allocate memory for decompression" on 32-bit 
perf with long perf.data.

On my side mmap() in perf_session__process_compressed_event() fails with ENOMEM,
due to exceeded memory limit for 32-bit applications.
This happens with or without Petr's patch.

As I can see, these mappings are only released in perf_session__delete().
We should think how to release them early.

On 02.12.2020 0:28, Alexei Budankov wrote:
> 
> Eventually sending to the proper Alexey's address.
> 
> On 02.12.2020 0:04, Alexei Budankov wrote:
>>
>> On 01.12.2020 22:09, Jiri Olsa wrote:
>>> On Mon, Nov 30, 2020 at 12:40:20PM +0100, Petr Malat wrote:
>>>> Hi Jiří,
>>>> were you able to reproduce the issue? I may also upload perf-archive
>>>> if that would help.
>>>
>>> oh yea ;-) seems like those 2 commits you reverted broke 32 bits
>>> perf for data files > 32MB
>>>
>>> but the fix you did does not work for Alexey's test he mentioned
>>> in the commit:
>>>
>>>       $ perf record -z -- some_long_running_workload
>>>       $ perf report --stdio -vv
>>>
>>> it's failing for me with:
>>>
>>> 	# ./perf report
>>> 	Couldn't allocate memory for decompression
>>> 	0xfe6f3a [0x60]: failed to process type: 81 [Operation not permitted]
>>> 	Error:
>>> 	failed to process sample
>>> 	# To display the perf.data header info, please use --header/--header-only options.
>>> 	#
>>>
>>> I think that's why here's special handling for compressed
>>> events, but I'll need to check on that in more detail,
>>> I was hoping for Alexey to answer ;-)
>>
>> Sorry for delay. Alexey Bayduraev could possibly help with that sooner.
>> Alexey, could you please follow up.
>>
>> Thanks,
>> Alexei

Regards,
Alexey

Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of PERF_RECORD_COMPRESSED records"

[Alexei Budankov]

Hi Alexey. Please see below.

On 02.12.2020 17:04, Bayduraev, Alexey V wrote:
> Hi,
> 
> I was able to reproduce "Couldn't allocate memory for decompression" on 32-bit 
> perf with long perf.data.
> 
> On my side mmap() in perf_session__process_compressed_event() fails with ENOMEM,
> due to exceeded memory limit for 32-bit applications.
> This happens with or without Petr's patch.
> 
> As I can see, these mappings are only released in perf_session__delete().
> We should think how to release them early.
> 
> On 02.12.2020 0:28, Alexei Budankov wrote:
>>
>> Eventually sending to the proper Alexey's address.
>>
>> On 02.12.2020 0:04, Alexei Budankov wrote:
>>>
>>> On 01.12.2020 22:09, Jiri Olsa wrote:
>>>> On Mon, Nov 30, 2020 at 12:40:20PM +0100, Petr Malat wrote:
>>>>> Hi Jiří,
>>>>> were you able to reproduce the issue? I may also upload perf-archive
>>>>> if that would help.
>>>>
>>>> oh yea ;-) seems like those 2 commits you reverted broke 32 bits
>>>> perf for data files > 32MB
>>>>
>>>> but the fix you did does not work for Alexey's test he mentioned
>>>> in the commit:
>>>>
>>>>       $ perf record -z -- some_long_running_workload
>>>>       $ perf report --stdio -vv
>>>>
>>>> it's failing for me with:
>>>>
>>>> 	# ./perf report
>>>> 	Couldn't allocate memory for decompression
>>>> 	0xfe6f3a [0x60]: failed to process type: 81 [Operation not permitted]
>>>> 	Error:
>>>> 	failed to process sample
>>>> 	# To display the perf.data header info, please use --header/--header-only options.
>>>> 	#
>>>>
>>>> I think that's why here's special handling for compressed
>>>> events, but I'll need to check on that in more detail,
>>>> I was hoping for Alexey to answer ;-)
>>>
>>> Sorry for delay. Alexey Bayduraev could possibly help with that sooner.
>>> Alexey, could you please follow up.

Next time please avoid top posting and reply in line.
For this specific case it is right here just below my
previous answer.

Thanks,
Alexei