linux-kernel - RE: [PATCH V3 04/12] perf mmap: introduce perf_mmap__read

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:   Tue, 9 Jan 2018 15:12:28 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     "acme@...nel.org" <acme@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "wangnan0@...wei.com" <wangnan0@...wei.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "yao.jin@...ux.intel.com" <yao.jin@...ux.intel.com>,
        "kernel-team@....com" <kernel-team@....com>
Subject: RE: [PATCH V3 04/12] perf mmap: introduce perf_mmap__read_done

> > > > >
> > > > > Also I guess the current code might miss some events since the head
> can
> > > be
> > > > > different between _read_init() and _read_done(), no?
> > > > >
> > > >
> > > > The overwrite mode requires the ring buffer to be paused during
> > > processing.
> > > > The head is unchanged between __read_init() and __read_done().
> > >
> > > Ah, ok then.  Maybe we could read the head once, and use it during
> > > processing.
> >
> > Yes, it only needs to read head once for overwrite mode.
> > But for non-overwrite, we have to read the head in every
> > perf_mmap__read_event(). Because the head is floating.
> > The non-overwrite is specially handled in patch 5/12 as well.
> 
> Right, I understand it for the non-overwrite mode.
> 
> But, for the overwrite mode, my concern was that it might be possible
> that it reads a stale head in __read_init() (even after it paused the
> ring buffer) and reads an update head in __read_done().  Then it's
> gonna miss some records.  I'm not sure whether it reads the same head
> in __read_init() and __read_done() by the pause.
>

The only scenario which may cause the different 'head' may be as below.
The 'rb->head' is updated in __perf_output_begin(), but haven’t been
assigned to 'pc->data_head' for perf tool. During this period, the 'paused'
is set and __read_init() reads head.
But this scenario never happens because of the ringbuffer lock.

Otherwise, I cannot imagine any other scenarios which may causes the
different 'head' in __read_init() and __read_done() with ringbuffer
paused. Please let me know if there is an example.

There would be some records miss. But it's only because the ringbuffer
is paused. The head should keep the same.

I also did some tests and dump the 'head' in __read_init() and
__read_done() with ringbuffer paused. I didn't see any difference either.

Thanks,
Kan
 
> Thanks,
> Namhyung
> 
> 
> > +	/* non-overwirte doesn't pause the ringbuffer */
> > +	if (!overwrite)
> > +		end = perf_mmap__read_head(map);