linux-kernel - Re: [PATCH] perf: fix perf_event

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: <BBB1A458-25CA-4C0F-AF04-18534D092142@fb.com>
Date:   Wed, 1 Mar 2023 23:16:32 +0000
From:   Song Liu <songliubraving@...a.com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     Song Liu <song@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] perf: fix perf_event_context->time



> On Mar 1, 2023, at 2:29 PM, Namhyung Kim <namhyung@...nel.org> wrote:
> 
> Hi Song,
> 
> On Tue, Feb 28, 2023 at 11:22 AM Song Liu <song@...nel.org> wrote:
>> 
>> Time readers rely on perf_event_context->[time|timestamp|timeoffset] to get
>> accurate time_enabled and time_running for an event. The difference between
>> ctx->timestamp and ctx->time is the among of time when the context is not
>> enabled. For cpuctx.ctx, time and timestamp should stay the same, however,
> 
> I'm not sure if it's correct.  The timestamp can go when the context is disabled
> for example, in ctx_resched() even if the NMI watchdog is enabled, right?

I think we do not disable EVENT_TIME for per cpu ctx? 

> 
>> it is not the case at the moment. To show this with drgn [1]:
>> 
>>    drgn 0.0.22 (using Python 3.8.6, elfutils 0.185, with libkdumpfile)
>>    For help, type help(drgn).
>>    ...
>>>>> ctx = per_cpu_ptr(prog['pmu'].pmu_cpu_context, 0).ctx
>>>>> ctx.timestamp * 1.0 / ctx.time
>>    (double)1.0385869134111765
>>>>> 
>> 
>> ctx->timestamp is about 4% larger than ctx.time. This issue causes time
>> read by perf_event_read_local() goes back in some cases.
> 
> I don't think the difference between the ctx.time and the timestamp
> itself is a problem.  I think the problem is it CAN update the timestamp
> (and the timeoffset) while the context is enabled.  Then depending on
> the timing, event times can return smaller values than before.

For per cpu ctx, I think timeoffset should stay the same (may not be zero 
though). 

Thanks,
Song