linux-kernel - Re: [PATCH v1 1/3] perf jit: Let convert_timestamp() to be backwards-compatible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Apr 2021 08:55:19 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Leo Yan <leo.yan@...aro.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Steve MacLean <Steve.MacLean@...rosoft.com>,
        Yonatan Goldschmidt <yonatan.goldschmidt@...nulate.io>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/3] perf jit: Let convert_timestamp() to be
 backwards-compatible

On 12/04/21 11:34 am, Leo Yan wrote:
> Commit d110162cafc8 ("perf tsc: Support cap_user_time_short for event
> TIME_CONV") supports the extended parameters for event TIME_CONV, but it
> broke the backwards compatibility, so any perf data file with old event
> format fails to convert timestamp.
> 
> For the backwards-compatibility, this patch checks the event size, if
> the event size confirms the extended parameters are supported in the
> event TIME_CONV, then copies these parameters.
> 
> Fixes: d110162cafc8 ("perf tsc: Support cap_user_time_short for event TIME_CONV")
> Signed-off-by: Leo Yan <leo.yan@...aro.org>

Perhaps could make use of a helper like the one described in my comments for patch 2.
Nevertheless:

Acked-by: Adrian Hunter <adrian.hunter@...el.com>

> ---
>  tools/perf/util/jitdump.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 9760d8e7b386..67b514c38a43 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -396,21 +396,32 @@ static pid_t jr_entry_tid(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  static uint64_t convert_timestamp(struct jit_buf_desc *jd, uint64_t timestamp)
>  {
> -	struct perf_tsc_conversion tc;
> +	struct perf_tsc_conversion tc = { 0 };
> +	struct perf_record_time_conv *time_conv = &jd->session->time_conv;
>  
>  	if (!jd->use_arch_timestamp)
>  		return timestamp;
>  
> -	tc.time_shift	       = jd->session->time_conv.time_shift;
> -	tc.time_mult	       = jd->session->time_conv.time_mult;
> -	tc.time_zero	       = jd->session->time_conv.time_zero;
> -	tc.time_cycles	       = jd->session->time_conv.time_cycles;
> -	tc.time_mask	       = jd->session->time_conv.time_mask;
> -	tc.cap_user_time_zero  = jd->session->time_conv.cap_user_time_zero;
> -	tc.cap_user_time_short = jd->session->time_conv.cap_user_time_short;
> +	tc.time_shift = time_conv->time_shift;
> +	tc.time_mult  = time_conv->time_mult;
> +	tc.time_zero  = time_conv->time_zero;
>  
> -	if (!tc.cap_user_time_zero)
> -		return 0;
> +	/*
> +	 * The event TIME_CONV was extended for the fields from "time_cycles"
> +	 * when supported cap_user_time_short, for backward compatibility,
> +	 * checks the event size and assigns these extended fields if these
> +	 * fields are contained in the event.
> +	 */
> +	if (time_conv->header.size >
> +		((void *)&time_conv->time_cycles - (void *)time_conv)) {
> +		tc.time_cycles	       = time_conv->time_cycles;
> +		tc.time_mask	       = time_conv->time_mask;
> +		tc.cap_user_time_zero  = time_conv->cap_user_time_zero;
> +		tc.cap_user_time_short = time_conv->cap_user_time_short;
> +
> +		if (!tc.cap_user_time_zero)
> +			return 0;
> +	}
>  
>  	return tsc_to_perf_time(timestamp, &tc);
>  }
>