Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 24 Nov 2020 15:36:45 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Petr Malat <oss@malat.biz>
Cc:     linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Alexey Budankov <alexey.budankov@linux.intel.com>
Subject: Re: [PATCH v2 1/3] Revert "perf session: Fix decompression of
 PERF_RECORD_COMPRESSED records"
Message-ID: <20201124143645.GD2088148@krava>
References: <20201124095923.3683-1-oss@malat.biz>
 <20201124102919.15312-1-oss@malat.biz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201124102919.15312-1-oss@malat.biz>
Precedence: bulk

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
>