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 <[email protected]>
> ---
> 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
>
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
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
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
>
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
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
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
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