linux-kernel - Re: [PATCH 1/2] perf mmap: Fix perf backward recording

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]

Message-ID: <4eeabf43-4de0-467c-04b0-85217040b036@huawei.com>
Date:   Wed, 1 Nov 2017 20:56:32 +0800
From:   "Wangnan (F)" <wangnan0@...wei.com>
To:     Jiri Olsa <jolsa@...hat.com>
CC:     Namhyung Kim <namhyung@...nel.org>, <linux-kernel@...r.kernel.org>,
        <kan.liang@...el.com>, <acme@...nel.org>, <kernel-team@....com>
Subject: Re: [PATCH 1/2] perf mmap: Fix perf backward recording



On 2017/11/1 20:39, Jiri Olsa wrote:
> On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote:
>
> SNIP
>
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index c6c891e154a6..27ebe355e794 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>>>>>                    if (*output == -1) {
>>>>>                            *output = fd;
>>>>> +                       if (evsel->attr.write_backward)
>>>>> +                               mp->prot = PROT_READ;
>>>>> +                       else
>>>>> +                               mp->prot = PROT_READ | PROT_WRITE;
>>>>> +
>>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
>>>> write_backward is
>>>> not set. If you want to delay the setting of mp->prot, you need to consider
>>>> both evlist->overwrite and evsel->attr.write_backward.
>>> I thought evsel->attr.write_backward should be set when
>>> evlist->overwrite is set.  Do you mean following case?
>>>
>>>     perf record --overwrite -e 'cycles/no-overwrite/'
>>>
>> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
>> said the concept of 'overwrite' and 'backward' is ambiguous.
>>
>> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
>> does is setting write_backward. Some testcases needs overwrite evlist.
> did not check deeply, but so why can't we do the below?
>
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4d9fc54b382..4643c487bd29 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
>   	struct record_opts *opts = &rec->opts;
>   	char msg[512];
>   
> -	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
> +	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite,
>   				 opts->auxtrace_mmap_pages,
>   				 opts->auxtrace_snapshot_mode) < 0) {

perf_evlist__mmap_ex's overwrite argument is used to control evlist->mmap.

Consider Namhyung's example:

   perf record --overwrite -e 'cycles/no-overwrite/'

In this case both evlist->mmap and evlist->backward_mmap would be set to overwrite.
'cycles' will be put into evlist->mmap, so it will be set to overwrite incorrectly.

Patch 2/2 clarifies these concepts. What we want is the flight recorder mode,
not only a read only ring buffer. Even flight recorder mode is selected, we can
still have a normal ring buffer which keep output data.

Thank you.