Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1533767600-7794-1-git-send-email-eranian@google.com> <20180809080721.GB19243@krava>
In-Reply-To: <20180809080721.GB19243@krava>
From:   Stephane Eranian <eranian@google.com>
Date:   Fri, 10 Aug 2018 01:21:18 -0700
Message-ID: <CABPqkBQEfoMDEqot1VbgExg1fU3+EEunzZqDFKSGmLKBhxNkwA@mail.gmail.com>
Subject: Re: [PATCH v2] perf ordered_events: fix crash in free_dup_event()
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>, mingo@elte.hu
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 9, 2018 at 1:07 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Aug 08, 2018 at 03:33:20PM -0700, Stephane Eranian wrote:
> > This patch fixes a bug in ordered_event.c:alloc_event().
> > An ordered_event struct was not initialized properly potentially
> > causing crashes later on in free_dup_event() depending on the
> > content of the memory. If it was NULL, then it would work fine,
> > otherwise, it could cause crashes such as:
>
> I'm now little puzzled what do we use this first event for..
> I can't see anything special about it, other than it's added
> on the list uninitialized ;-)
>
> it seems to work properly when we ditch it.. might be some
> prehistoric leftover or I'm terribly missing something
>
You need to keep track of the buffers to free. You do not free the
ordered_event structs
individually. For each oe->buffer, you need one free(). Each buffer is
put in the to_free
list. But to link it into the list it needs a list_head. This is what
buffer[0] is used for.
But the logic is broken in  ordered_events__free(). It does not free individual
ordered_event structs, but a buffer with many. Yet, it is missing
freeing all of the duped
events.

 void ordered_events__free(struct ordered_events *oe)
{
        while (!list_empty(&oe->to_free)) {
                struct ordered_event *buffer;

                buffer = list_entry(oe->to_free.next, struct
ordered_event, list);
                list_del(&buffer->list);
---->         free_dup_event(oe, event->event);
                free(buffer);
        }
}
This only frees the dup_event of buffer[0] which we know is NULL (well, now).
It needs to walk all the entries in buffer[] to free buffer[x].event.

I think the goal was likely to avoid adding another list_head field to
each ordered_event
and instead use one per allocated buffer.
This is very convoluted and prone to errors and we are seeing right
now. This should
be cleaned. So either you add a list_head to ordered_event or you
would buffer[x] in
ordered_events_free().

At this point, this is my understanding.
Do you agree?


>
> thanks,
> jirka
>
>
> ---
> diff --cc tools/perf/util/ordered-events.c
> index bad9e0296e9a,0e837b0b8582..000000000000
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@@ -119,12 -119,8 +119,9 @@@ static struct ordered_event *alloc_even
>                 pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n",
>                    oe->cur_alloc_size, size, oe->max_alloc_size);
>
>  +              oe->cur_alloc_size += size;
> -               list_add(&oe->buffer->list, &oe->to_free);
> -
> -               /* First entry is abused to maintain the to_free list. */
> -               oe->buffer_idx = 2;
> -               new = oe->buffer + 1;
> +               oe->buffer_idx = 1;
> +               new = oe->buffer;
>         } else {
>                 pr("allocation limit reached %" PRIu64 "B\n", oe->max_alloc_size);
>         }