linux-kernel - Re: [PATCH] perf/x86/amd: check event before enable to avoid GPF

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, 4 Jun 2024 18:30:33 +0530
From: Ravi Bangoria <ravi.bangoria@....com>
To: George Kennedy <george.kennedy@...cle.com>
Cc: harshit.m.mogalapalli@...cle.com, peterz@...radead.org, mingo@...hat.com,
 acme@...nel.org, namhyung@...nel.org, mark.rutland@....com,
 alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
 adrian.hunter@...el.com, kan.liang@...ux.intel.com, tglx@...utronix.de,
 bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 ravi.bangoria@....com
Subject: Re: [PATCH] perf/x86/amd: check event before enable to avoid GPF

>>> Events can be deleted and the entry can be NULL.
>> Can you please also explain "how".
> It looks like x86_pmu_stop() is clearing the bit in active_mask and setting the events entry to NULL (and doing it in the correct order) for the same events index that amd_pmu_enable_all() is trying to enable.

Right, so how can it disable the exact same event which it's about to
enable?

If it's happening because of some race condition, we need to find and fix
it. For ex: https://git.kernel.org/torvalds/c/baa014b9543c8

>>> Check event for NULL in amd_pmu_enable_all() before enable to avoid a GPF.
>>> This appears to be an AMD only issue.
>>>
>>> Syzkaller reported a GPF in amd_pmu_enable_all.
>> Can you please provide a bug report link? Also, any reproducer?
> The Syzkaller reproducer can be found in this link:
> https://lore.kernel.org/netdev/CAMt6jhyec7-TSFpr3F+_ikjpu39WV3jnCBBGwpzpBrPx55w20g@mail.gmail.com/T/#u
>>
>>> @@ -760,7 +760,8 @@ static void amd_pmu_enable_all(int added)
>>>           if (!test_bit(idx, cpuc->active_mask))
>>>               continue;
>>>   -        amd_pmu_enable_event(cpuc->events[idx]);
>>> +        if (cpuc->events[idx])
>>> +            amd_pmu_enable_event(cpuc->events[idx]);
>> What if cpuc->events[idx] becomes NULL after if (cpuc->events[idx]) but
>> before amd_pmu_enable_event(cpuc->events[idx])?
> Good question, but the crash has not reproduced with the proposed fix in hours of testing. It usually reproduces within minutes without the fix.

It does not reproduce with the patch because that if() condition makes the
race window smaller?

Thanks,
Ravi