linux-kernel - Re: [PATCH] perf tools: Fix indexing for decoder packet queue

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, 28 May 2018 11:13:59 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Robert Walker <robert.walker@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        LAK <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf tools: Fix indexing for decoder packet queue

On Fri, May 25, 2018 at 05:10:54PM -0600, Mathieu Poirier wrote:
> The tail of a queue is supposed to be pointing to the next available slot
> in a queue.  In this implementation the tail is incremented before it is
> used and as such points to the last used element, something that has the
> immense advantage of centralizing tail management at a single location
> and eliminating a lot of redundant code.
>
> But this needs to be taken into consideration on the dequeueing side where
> the head also needs to be incremented before it is used, or the first
> available element of the queue will be skipped.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
> ---
>  tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index c8b98fa22997..4d5fc374e730 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -96,11 +96,19 @@ int cs_etm_decoder__get_packet(struct cs_etm_decoder *decoder,
>   /* Nothing to do, might as well just return */
>   if (decoder->packet_count == 0)
>   return 0;
> + /*
> + * The queueing process in function cs_etm_decoder__buffer_packet()
> + * increments the tail *before* using it.  This is somewhat counter
> + * intuitive but it has the advantage of centralizing tail management
> + * at a single location.  Because of that we need to follow the same
> + * heuristic with the head, i.e we increment it before using its
> + * value.  Otherwise the first element of the packet queue is not
> + * used.
> + */
> + decoder->head = (decoder->head + 1) & (MAX_BUFFER - 1);
>
>   *packet = decoder->packet_buffer[decoder->head];
>
> - decoder->head = (decoder->head + 1) & (MAX_BUFFER - 1);
> -

I tested this patch and confirmed it can work well with python
decoding script:

Tested-by: Leo Yan <leo.yan@...aro.org>

Actually, I have another idea for this fixing, seems to me
the unchanged code has right logic for decoder->head, and I think this
issue is more related with incorrect initialization index.  So we can
change the initialization index for decoder->head as below.  How about
you think for this?

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
index c8b98fa..b133260 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -249,7 +249,7 @@ static void cs_etm_decoder__clear_buffer(struct
cs_etm_decoder *decoder)
 {
        int i;

-       decoder->head = 0;
+       decoder->head = 1;
        decoder->tail = 0;
        decoder->packet_count = 0;
        for (i = 0; i < MAX_BUFFER; i++) {

Thanks,
Leo Yan

>   decoder->packet_count--;
>
>   return 1;
> --
> 2.7.4
>