linux-kernel - Re: [V14 3/8] drivers: perf: arm_pmuv3: Enable branch stack sampling framework

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] [day] [month] [year] [list]

Date:   Thu, 30 Nov 2023 09:28:23 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     James Clark <james.clark@....com>
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
        Marc Zyngier <maz@...nel.org>,
        Suzuki Poulose <suzuki.poulose@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-perf-users@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        will@...nel.org, catalin.marinas@....com, mark.rutland@....com
Subject: Re: [V14 3/8] drivers: perf: arm_pmuv3: Enable branch stack sampling
 framework



On 11/14/23 22:40, James Clark wrote:
> 
> On 14/11/2023 05:13, Anshuman Khandual wrote:
> [...]
>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
>> index d712a19e47ac..76f1376ae594 100644
>> --- a/drivers/perf/arm_pmu.c
>> +++ b/drivers/perf/arm_pmu.c
>> @@ -317,6 +317,15 @@ armpmu_del(struct perf_event *event, int flags)
>>  	struct hw_perf_event *hwc = &event->hw;
>>  	int idx = hwc->idx;
>>  
>> +	if (has_branch_stack(event)) {
>> +		WARN_ON_ONCE(!hw_events->brbe_users);
>> +		hw_events->brbe_users--;
>> +		if (!hw_events->brbe_users) {
>> +			hw_events->brbe_context = NULL;
>> +			hw_events->brbe_sample_type = 0;
>> +		}
>> +	}
>> +
>>  	armpmu_stop(event, PERF_EF_UPDATE);
>>  	hw_events->events[idx] = NULL;
>>  	armpmu->clear_event_idx(hw_events, event);
>> @@ -333,6 +342,22 @@ armpmu_add(struct perf_event *event, int flags)
>>  	struct hw_perf_event *hwc = &event->hw;
>>  	int idx;
>>  
>> +	if (has_branch_stack(event)) {
>> +		/*
>> +		 * Reset branch records buffer if a new task event gets
>> +		 * scheduled on a PMU which might have existing records.
>> +		 * Otherwise older branch records present in the buffer
>> +		 * might leak into the new task event.
>> +		 */
>> +		if (event->ctx->task && hw_events->brbe_context != event->ctx) {
>> +			hw_events->brbe_context = event->ctx;
>> +			if (armpmu->branch_reset)
>> +				armpmu->branch_reset();
> What about a per-thread event following a per-cpu event? Doesn't that
> also need to branch_reset()? If hw_events->brbe_context was already
> previously assigned, once the per-thread event is switched in it skips
> this reset following a per-cpu event on the same core.

Right, guess it is real a possibility. How about folding in something like ..

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 76f1376ae594..15bb80823ae6 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -343,6 +343,22 @@ armpmu_add(struct perf_event *event, int flags)
        int idx;
 
        if (has_branch_stack(event)) {
+               /*
+                * Reset branch records buffer if a new CPU bound event
+                * gets scheduled on a PMU. Otherwise existing branch
+                * records present in the buffer might just leak into
+                * such events.
+                *
+                * Also reset current 'hw_events->brbe_context' because
+                * any previous task bound event now would have lost an
+                * opportunity for continuous branch records.
+                */
+               if (!event->ctx->task) {
+                       hw_events->brbe_context = NULL;
+                       if (armpmu->branch_reset)
+                               armpmu->branch_reset();
+               }
+
                /*
                 * Reset branch records buffer if a new task event gets
                 * scheduled on a PMU which might have existing records.